-
Notifications
You must be signed in to change notification settings - Fork 169
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
fix(helm): Check prior vendoring of helm charts #402
Conversation
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.
Thanks for this awesome contribution! Got one small question, then this is good to go :)
@@ -199,3 +206,14 @@ func parseReq(s string) (*Requirement, error) { | |||
Version: *ver, | |||
}, nil | |||
} | |||
|
|||
// parseReqName parses a name from a string of the format `repo/name` or `name` | |||
func parseReqName(s 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.
It seems like parseReqName
is only ever called with an argument of the form name@repo
as r.Chart
looks like this.
Any reason this function also handles plain name
?
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'm just being defensive in the case a chart name didn't have a repo/
prefix, I'm not sure how the helm community does it across the board.
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.
@sh0rez Does that make sense? I'm happy to simplify it if that case won't happen. Just let me know.
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 use of repo/name
promotes making a clear distinction which Chart one refers to, I would be in favor of enforcing 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.
Gotcha. I removed the check.
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.
Thanks @justinwalz!
b144399
to
a8a9594
Compare
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 🚀
Fixes #401