-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat(clone-image): make clone image configurable #755
Conversation
Codecov Report
@@ Coverage Diff @@
## main #755 +/- ##
==========================================
+ Coverage 54.38% 54.39% +0.01%
==========================================
Files 225 225
Lines 16184 16188 +4
==========================================
+ Hits 8802 8806 +4
Misses 6989 6989
Partials 393 393
|
also readied go-vela/worker#417 to fix broken tests once this is sourced in worker code. docs PR incoming as well |
docs draft PR opened |
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.
NOTE: Thanks for this! Code LGTM so requesting changes for visibility purposes (i.e. Trailer.io)
Do we think we need to "validate" the value provided as the clone image?
I know we already ensure a value is provided which is a good thing 👍
But I'm wondering if we want to "force" people to use target/vela-git
?
i.e. if !strings.Contains(c.String("clone-image"), "target/vela-git") { return err }
I'm leaning towards NOT forcing and leaving this PR as-is.
However, I thought it was worth opening the discussion for it.
@jbrockopp forgot about the comment on this. i'm also leaning towards not doing that. as suggested, you could trivially bypass by using |
Yeah, thinking it would be a security enhancement to ensure a "valid" clone image is used. That being said, I worry it would limit organizations from creating their own customized clone image. Let's plan to roll forward as is and we can always consider it in the future 👍 |
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
closes go-vela/community#456
adds a
VELA_CLONE_IMAGE
configuration to the server that allows a Vela administrator to define the default clone image to use for the Vela injected clone step.