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
ADR 46: Build a common Task Runner image #217
ADR 46: Build a common Task Runner image #217
Changes from all commits
3927962
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.
Should we have any recommendation on when it makes sense to create these specialized images? Or how narrowly focused they should be?
I proposed adding an additional endpoint to the build-trusted-artifacts image, but the contract team deemed that it would be better to put it somewhere else.
@zregvart @lcarva , fyi. I feel like this ADR is the opposite direction of what you were proposing.
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 recommendation could be this: if you're thinking of including a tool in the Task Runner image and it does not follow semver and/or isn't easily installable as a standalone tool, don't include it.
As an example, that would mean these images should be kept standalone
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.
Yeah, because of the nature of the build-trusted-artifacts image/repo. The proposal here is to create a multi-tool repo/image, which by definition has a much wider scope.
IMO the current state of those tools reflect the current state of getting code to run in the Konflux tasks. If we have a well-defined list of requirement for what it takes to be added to the runner image, then I expect these tools to be motivated to abide to them, especially if they are reasonable.
I expect a tool like oras to be compiled from source when building the runner image. Much like it's current done in https://github.com/konflux-ci/oras-container. The source is referenced via a git submodule. Would this be a reasonable approach for all of the tools?
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.
With git submodules, it's kind of hard to to follow versions (or even to know what version you are building). Renovate just bumps them to the latest commit in main.
For tools written in Go, I was thinking of experimenting with a
tools.go
-based approach. We wouldn't include them as submodules, just declare them as dependencies. That gets you proper versioning for free. (That probably doesn't belong in this ADR though, as long as the image knows exactly what version of each tool it includes, it's fine)💯
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.
Added the suggested requirements for tool inclusion
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.
Also (maybe a stretch) there is a higher chance that the image will already be cached in the node.
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 realized that "Pipelines pull each image at most once" is not true, all the Tasks in a Pipeline don't usually run on the same node. I still think the common image would reduce overall image pull rates though. Rephrased