Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow setting custom images for addons #10111

Merged
merged 11 commits into from
Feb 1, 2021
Merged

Allow setting custom images for addons #10111

merged 11 commits into from
Feb 1, 2021

Conversation

lingsamuel
Copy link
Contributor

@lingsamuel lingsamuel commented Jan 8, 2021 •
edited
Loading

Signed-off-by: Ling Samuel [email protected]

Currently addon image names are hardcoded, see #10063.

This PR move images to golang instead of yaml (currently migrated: ingress). To fully support #10063, we need to migrate all yaml template to the format this PR used.

Fixes #10063

Changes:

  • name all images in yaml, store them in Addon struct
  • add param --images for addons enable, format is: ImageName=Image,ImageName2=Image2
  • add param --registries for addons enable, format is: ImageName=Registry,ImageName2=Registry2. Empty is okay (will use docker.io).
  • add command addon images to view how many images the addon requires, and images name used above
  • fix several ImageRepository mistakes in yaml template

Examples:
Custom 1 image (newest images output see below):
image

Custom 2 images:
image

Custom image and registry:
image

addons images ingress:
image

Note:

  • ingress yaml using different version kube-webhook-certgen, I am not sure if it's by design, so I keep this as is.
  • OLM image quay.io/operator-framework/olm@sha256:0d15ffb5d10a176ef6e831d7865f98d51255ea5b0d16403618c94a004d049373 is 0.14.1:
    image

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 8, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @lingsamuel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 8, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 8, 2021
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@lingsamuel lingsamuel changed the title Manage addon images in code, support custom addon images Support custom addon images Jan 8, 2021
@lingsamuel
Copy link
Contributor Author

/cc @tstromberg

medyagh
medyagh requested changes Jan 8, 2021
View reviewed changes
cmd/minikube/cmd/config/images.go Outdated Show resolved Hide resolved
cmd/minikube/cmd/config/images.go Outdated Show resolved Hide resolved
deploy/addons/ingress/ingress-dp.yaml.tmpl Outdated Show resolved Hide resolved
medyagh
medyagh requested changes Jan 8, 2021
View reviewed changes
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do u mind adding an example output of an addon with multiple images and replacing one of the images in the addon ?

would it support if the user wants to replace only one of the images? could we see an example?

@lingsamuel lingsamuel changed the title Support custom addon images [WIP] Support custom addon images Jan 11, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2021
@lingsamuel
Copy link
Contributor Author

do u mind adding an example output of an addon with multiple images and replacing one of the images in the addon ?

would it support if the user wants to replace only one of the images? could we see an example?

Added. Currently I migrated ingress addon, requires 3 images, user can custom all/some of these images.
The further work would be migrate other addons.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2021
@lingsamuel lingsamuel changed the title [WIP] Support custom addon images Support custom addon registries/images Jan 11, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2021
@lingsamuel lingsamuel changed the title Support custom addon registries/images [WIP] Support custom addon registries/images Jan 11, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2021
@lingsamuel lingsamuel changed the title [WIP] Support custom addon registries/images Support custom addon registries/images Jan 12, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2021
@lingsamuel

This comment has been minimized.

Sign in to view
@lingsamuel
Copy link
Contributor Author

/cc @medyagh
finished

medyagh
medyagh requested changes Jan 13, 2021
View reviewed changes
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for updating the PR, @lingsamuel lets add index to the table too

cmd/minikube/cmd/config/images.go Show resolved Hide resolved
@medyagh medyagh mentioned this pull request Jan 20, 2021
Open
@medyagh
Copy link
Member

medyagh commented Jan 20, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 20, 2021
@lingsamuel
Copy link
Contributor Author

Seems like all failing tests are flaky.

@medyagh
Copy link
Member

medyagh commented Jan 26, 2021

@lingsamuel do u mind pulling upstream master

@lingsamuel
Manage addon images in code, support custom addon images
b949034 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
update docs
a49d915 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Remove default image from yaml, fix empty images warning
7b2d1a6 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Implement all addons
de3c901 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Make registry configurable
76833fd 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Fix docker.io/ prefix
52f0e49 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Ignore empty params
0468587 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Allow empty custom registry
6756336
@lingsamuel
addons custom images/registries example article
c9f4cb8 
Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Addon custom registry:
101d39f 
* fix missing suffix when using global image repo
* support override global image repo
* change all imagePullPolicy to IfNotPresent
* fix empty global image repo suffix

Signed-off-by: Ling Samuel <[email protected]>
@lingsamuel
Copy link
Contributor Author

@medyagh done
🤔 the bot didn't notify me need rebase

@medyagh
Copy link
Member

medyagh commented Feb 1, 2021

@lingsamuel sorry for the delay in reviewing this PR, and this looks good to me. thank you for adding this feature

@medyagh medyagh self-requested a review February 1, 2021 20:10
@medyagh medyagh changed the title Support custom addon registries/images Allow adding custom images for addons Feb 1, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lingsamuel, medyagh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2021
@medyagh medyagh changed the title Allow adding custom images for addons Allow setting custom images for addons Feb 1, 2021
@medyagh medyagh merged commit 6881977 into kubernetes:master Feb 1, 2021
@clamprecht01 clamprecht01 mentioned this pull request Mar 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medyagh medyagh medyagh approved these changes

@josedonizetti josedonizetti Awaiting requested review from josedonizetti

@RA489 RA489 Awaiting requested review from RA489

@tstromberg tstromberg Awaiting requested review from tstromberg

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make addon yaml templates image name configurable
4 participants
@lingsamuel @k8s-ci-robot @minikube-bot @medyagh