-
Notifications
You must be signed in to change notification settings - Fork 6.1k
fix: Make usage of quotes for templating consistent #22605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: raweber <[email protected]>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
The user only needs to make sure that the field is a YAML string. Beyond that, Argo has no opinion on (or knowledge of) the string delimitation. |
@crenshaw-dev alright, thank you! But then I think it would be wise to consistently use single or double quotes across the argocd documentation to reduce room for speculation (like for me). If you approve of this, I am happy to create an extensive PR to update the usage examples across the ArgoCD docs. |
I don't feel strongly either way 🙂 |
@@ -372,7 +372,7 @@ spec: | |||
source: | |||
repoURL: https://github.com/argoproj/argo-cd.git | |||
targetRevision: HEAD | |||
path: "{{.values.base_dir}}/apps/guestbook" | |||
path: '{{.values.base_dir}}/apps/guestbook' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path: '{{.values.base_dir}}/apps/guestbook' | |
path: '{{.values.base_dir}}/apps/guestbook' |
I personally think it isn't necessary. If we change it to single dash, this gives the user a perspective that only single dash could be used (unless they try double dash too). Having both sort of examples makes sense. Although you're correct about consistency around docs but I don't think it's such a big change that needs to be consistent across all docs. Having said that, I'm proposing to revert the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that having both examples makes sense. But if I got confused by it, others will, too. It's literally the only place on the whole page where double quotes are used around templated values 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I'm still not sure if it's required. Instead adding it as a note would make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I definitely agree in regards to the note! Would be good to put it somewhere globally? How about in the FAQ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add it in the docs rather than FAQ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, I'm just very busy this week. But it's on my list!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing as a +1 to Nitish. Folks need to understand YAML in order to use it effectively. I don't think we can overcome that lack of understanding via our docs. |
Checklist:
I got confused with the usage of single and double quotes. I don't think there is any significance in using double quotes around the
.values
templating value, is there? In line 237, single quotes are being used, too. So I guess this would be good for consistency. In general: I am wondering: When do we use single/double quotes? I didn't find any difference in how ArgoCD expands variables etc within single vs double quotes.