Skip to content
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

update crane mutate annotation/label args to allow commas in label values #1178

Merged
merged 1 commit into from
Nov 15, 2021

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Nov 14, 2021

With the current argument type, a common in a label value (a reasonable thing to have) will be split and parsed as a separate label, and the client errors. This commit updates the flag to StringToStringVarP so we start with a map[string]string off the bat, and do not need to do special parsing beyond checking for empty strings.

I also added some additional notes to the crane mutate doc page for points that weren't clear to me - 1. that the image needed to exist in a registry and would be changed there, and 2. a full example for using the command.

This will close #1177

Signed-off-by: vsoch vsoch@users.noreply.github.com

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2021

Codecov Report

Merging #1178 (e89b699) into main (f30efdd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1178   +/-   ##
=======================================
  Coverage   75.23%   75.23%           
=======================================
  Files         108      108           
  Lines        7855     7855           
=======================================
  Hits         5910     5910           
  Misses       1379     1379           
  Partials      566      566           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f30efdd...e89b699. Read the comment docs.

@vsoch vsoch force-pushed the fix/crane-mutate-label branch 2 times, most recently from 6053095 to 61089c5 Compare November 14, 2021 18:43
@vsoch
Copy link
Contributor Author

vsoch commented Nov 14, 2021

How are the client docs updated? I found the hack/presubmit.sh to validate but am not sure how to re-generate the docs for the updated client.

@imjasonh
Copy link
Collaborator

How are the client docs updated? I found the hack/presubmit.sh to validate but am not sure how to re-generate the docs for the updated client.

./hack/update-codegen.sh

Specifically the last line: https://github.com/google/go-containerregistry/blob/main/hack/update-codegen.sh#L52

@vsoch
Copy link
Contributor Author

vsoch commented Nov 14, 2021

Ahh I literally just ran that! Asked too soon :P

// splitKeyVals splits key value pairs which is in form hello=world
func splitKeyVals(kvPairs []string) (map[string]string, error) {
// validateKeyVals ensures no values are empty
func validateKeyVals(kvPairs map[string]string) (map[string]string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could just return error, and not bother creating a copy of the input kv map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set! I like that much better - we can use "labels" and "annotations" off the bat.

…lues

With the current argument type, a common in a label value (a reasonable thing to have)
will be split and parsed as a separate label, and the client errors. This commit updates
the flag to StringToStringVarP so we start with a map[string]string off the bat, and do not
need to do special parsing beyond checking for empty strings.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Copy link
Collaborator

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍

@vsoch
Copy link
Contributor Author

vsoch commented Nov 15, 2021

uhoh looks like we've hit some pull limit?
image

The presubmit.sh worked locally!

@imjasonh
Copy link
Collaborator

uhoh looks like we've hit some pull limit? image

The presubmit.sh worked locally!

Weird. That doesn't look like the rate limit error message, just general slowness. Retrying, let's hope 🤞

@imjasonh imjasonh merged commit 88f6c00 into google:main Nov 15, 2021
@vsoch
Copy link
Contributor Author

vsoch commented Nov 15, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crane mutate for label does not like commas
3 participants