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.
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
SHIP-0022: Build Strategy Volumes #23
SHIP-0022: Build Strategy Volumes #23
Changes from all commits
ca20808
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
WRT this, could we not create a first class CRD like
BuildVolume
to encapsulate what you currently have for each entry under yourvolumes
array in the strategy spec. Then developers could query just on those.Seems like labeling to indicate which volumes applied to which strategies could help narrow the query for developers.
The developer just needs to know the name of the strategy.
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 approach would make sense if a
BuildVolume
would be shared across build strategies, which I don't think is the case.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.
Hmm ... interesting, I was thinking the opposite. That the labels would be a way to associate a BuildVolume CRD with a particular build strategy.
i.e. you have 20 BuildVolumes. 2 are used by strategy A, 6 by strategy B. Sure, maybe you could use naming conventions to draw associations. But the value of a given label for a BuildVolume could be the strategy that uses it.
Separate point on the sharing across strategies. I can see the notion of not sharing build volumes across strategies being the more likely landing spot. But I'm not convinced that there will not be a non-trivial amount of image layer caching scenarios that span more than one strategy. For example, I could see more than one maven based build strategy existing, and both want to leverage a volume that houses some very common maven downloaded jar files.
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.
Practically speaking, I don't think we want to design an API that encourages lots of build strategies to be created. Users shouldn't need to create separate build strategies for, say, kaniko because one uses an image layer cache, and another doesn't.
I'm going to add a more concrete example in the design.
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.
Hmm ... I suppose there is "encourages" vs. "accounting for the fact that our users will do it regardless of what we encourage".
All that said, the reality is we don't have tangible user feedback in this space both here with shipwright of course, as well as with openshift, given image layer caching as we both know is one of the remaining features we have not gotten to yet prior to calling that function feature complete. So end of the day, any arguments are speculative ones IMO at this point.
Your
Example - Buildah with Image Cache
is a useful addition to the SHIP/EP. I'll take that as a positive outcome of our thread here. Aside from having no immediate qualms / I'm good with it, in theory, we could "extend" at the k8s volume source level if need be. i.e.if user feedback drove us down the path I'm speculating on. Certainly, our recent openshift entitlements forays have educated us on the pros (and cons) of providing CSI volumes and the like :-)
I wouldn't mind @adambkaplan you leaving this discussion thread open and not resolved so it is easier to find in the future if need be, and we revisit the reviews that lead to your SHIP merging, but otherwise, consider me OK with the outcome here.
thanks
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 adding a brief write-up of this in the "Alternatives" section as well.
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 brief write is fine with me @adambkaplan thanks