-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Convert git tag into proper docker tag #3407
Conversation
Codecov Report
|
pkg/skaffold/build/tag/git_commit.go
Outdated
return fmt.Sprintf("%s:%s", imageName, ref), nil | ||
} | ||
|
||
func gitTagToDockerTag(text string) string { |
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.
Nit: I don't think there is anything got specific about this - and it doesn't say what it does: escapeAndTruncateAsTag
?
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.
Or sanitizeTag
could be another option
if len(withAuthorizedPrefix) > 128 { | ||
return withAuthorizedPrefix[0:128] | ||
} | ||
|
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 would also add a warning or maybe debug message for visibility when we do change the tag - when tags start to overlap accidentally,this can be a useful hint
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.
LGTM with small nits
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Fixed all the nits |
Fixes #3344