Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Service and Revision labels #342
Service and Revision labels #342
Changes from 13 commits
b9c6e52
158202f
ac92237
f1f0147
b9992db
578a4d5
664f703
8cc9d57
2a3fb20
7cd588e
6ba099b
da2029d
202f086
3f7d0ca
29dc7d5
a962ae0
a80f54f
4a9c6da
dc17266
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The question is whether the labels of the service or the labels of the revision template which should be set.
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 think this should probably be decided in #328. I like the simplicity of
--label
setting both, but not sure if preventing users from setting separate labels on services and revision templates would be an issue.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.
My gut feeling is that people most often want to set labels for their pods, not the service. So we could settle on something like that:
--label mylabel=myvalue
would set it on both, the service and the pod--label ^mylabel=myvalue
would set it only on theService
(the "outer" entity)--label :mylabel=myvalue
would set it only on the revision template (the "inner" entity)That would cover 90% of all use case by setting both, but allows for tuning, too. Not sure about te concrete characters (one should also check whether there are special shell meanings so that those chars can be used unescaped), but using any character which can not be used within label keys should be cool.
wdyt ?
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.
It seems at that point there might as well be separate
--revision-label
,--service-label
, and--label
flags. It would be much more explicit both in documentation and I think code that way. Plus, special characters change (slightly) across different shell environments and if this list is any indication, it may not be worth the time and effort to find and confirm which characters are valid in a reasonable number of shell environments.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 too like the simplicity of
—label
for all labels as the object is already decided in the command group. Adding more complexity and notations to remember is unnecessary as it complicates the “language” of the CLI IMO.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.
This is always a tradeoff between options bloat and being too obfuscated. Currently, there is a tendency to add 1-5 new options per new features, and I'm doing my best to fight against the bloat. At least asking, whether there could be a compromise and whether all those options are needed (e.g. see #345 (comment) or #326 (review) or #282 (review) how to reduce the UX surface and so the cognitive load).
Of course, we should no go over the top, and using special chars like that might be the case here (although
^
and:
are safe shell chars so no need to escape).What's about that compromise: Let's implement
--label
to label both, service and template and wait on feedback if the other use cases (individual labelling of serving/template) are actually requested ?From a single feature's PR POV adding 3 new options might not be a big deal, but its the sum which might lead to multi page help outputs which are really overwhelming (especially when alphabetically ordered, and --revision-label, --sevice-label and --label will apear on totally different positions in a list of 50+ options)
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.
OK on compromise!
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.
Aside: I'm starting to think we should implement our own flags groupings, which might help with this.
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.
spf13/cobra#836 has some discussion on flag groupings in help text. Unfortunately, it doesn’t look like there’s currently first class support for it.