-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add a minimal nop image #3014
Add a minimal nop image #3014
Conversation
/kind cleanup |
This satisfies Tekton's need for a minimal image that will simply exit immediately (to gracefully stop sidecars), and for a minimal image that will run indefinitely (to power the Affinity Assistant), and will be owned and released by Tekton unlike the tianon/true and nginx images it replaces.
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.
SGTM, just one question
nopImage = flag.String("nop-image", "tianon/true", "The container image used to stop sidecars") | ||
affinityAssistantImage = flag.String("affinity-assistant-image", "nginx", "The container image used for the Affinity Assistant") | ||
gitImage = flag.String("git-image", "override-with-git:latest", | ||
nopImage = flag.String("nop-image", "override-with-nop:latest", "The container image used to stop sidecars") |
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.
Does nop image runs indefinitely ?
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 image runs in two modes, one which exits immediately (to stop sidecars) and another that runs indefinitely (to support Affinity Assistant). So it can run indefinitely, but for sidecar-stopping it won't, and will just exit immediately.
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.
ohhhhhhhhh I didn't know about that 🙃
Sounds very good then 😝
/lgtm |
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 looks good!
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlpettersson, sbwsg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Nice! |
Will this also need some stuff in the tekton directory to ensure the nop image is built along with our other internal images? I'm still not super familiar with this stuff but I would guess it needs an entry in |
This should ensure that the nop-image is built and released alongside other utility images. Followup from tektoncd#3014
Good catch! |
This should ensure that the nop-image is built and released alongside other utility images. Followup from #3014
This should ensure that the nop-image is built and released alongside other utility images. Followup from tektoncd#3014
This satisfies Tekton's need for a minimal image that will simply exit
immediately (to gracefully stop sidecars), and for a minimal image that
will run indefinitely (to power the Affinity Assistant), and will be
owned and released by Tekton unlike the tianon/true and nginx images it
replaces.
Fixes #2714
Fixes #2640
/cc @sbwsg @jlpettersson
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes