-
Notifications
You must be signed in to change notification settings - Fork 261
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
Merged
knative-prow-robot
merged 19 commits into
knative:master
from
aaron-lerner:service-labels
Aug 14, 2019
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
b9c6e52
Add support for service labels with --label or -l
aaron-lerner 158202f
Add tests
aaron-lerner ac92237
Fix typo
aaron-lerner f1f0147
Move MapToArray helper method to separate file
aaron-lerner b9992db
Allow unsetting labels by passing empty value
aaron-lerner 578a4d5
Fix test
aaron-lerner 664f703
Add test case for label removal
aaron-lerner 8cc9d57
Wrap error message with flag
aaron-lerner 2a3fb20
Update docs to include --label
aaron-lerner 7cd588e
Update labels on both services and revisions
aaron-lerner 6ba099b
Add some test cases around weirder user input
aaron-lerner da2029d
Change unset behavior and allow setting empty env and label
aaron-lerner 202f086
Update docs for new unset behavior
aaron-lerner 3f7d0ca
Make single keys to map to empty values
aaron-lerner 29dc7d5
Move helper to util
aaron-lerner a962ae0
Use assert.DeepEqual
aaron-lerner a80f54f
Use new mock test + check both service and revision
aaron-lerner 4a9c6da
Merge remote-tracking branch 'upstream/master' into service-labels
aaron-lerner dc17266
Use new method and correct year
aaron-lerner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.