-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Avoid MetalLB speaker image download when MetalLB speaker is disabled #9248
Avoid MetalLB speaker image download when MetalLB speaker is disabled #9248
Conversation
Hi @unai-ttxu. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
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.
@unai-ttxu Thank you
/ok-to-test
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floryut, unai-ttxu 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 |
@unai-ttxu Could you rebase this pull request based on the latest master branch to pass those test jobs? |
0e240c5
to
e7d0a2a
Compare
Done! |
e7d0a2a
to
c077492
Compare
molecule_docker job is failed like:
So we might need to move |
21f316e
to
dd97c80
Compare
Done! Thanks for the help @oomichi 😄 |
@@ -409,6 +409,7 @@ ingress_alb_enabled: false | |||
cert_manager_enabled: false | |||
expand_persistent_volumes: false | |||
metallb_enabled: false | |||
metallb_speaker_enabled: true |
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.
In this case(default true
), metallb_speaker would be downloaded even if metallb is disabled. Right?
I guess it is better to use metallb_enabled
value for metallb_speaker_enabled by default.
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.
Good catch, you're wright!
I also decided to refactor all metallb_speaker_enabled
references to be set to metallb_speaker_enabled: "{{ metallb_enabled }}"
since enabling metallb_speaker_enabled
only make sense when enabling metallb_enabled
.
What do you think about it?
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.
Thanks for doing that, I think that is a nice idea.
/lgtm
…kubernetes-sigs#9248) * Avoid MetalLB speaker image download when metallb_speaker_enabled is set to * Move metallb_speaker_enabled var to allow outside metalLB role references * Move metallb_speaker_enabled var to allow outside metalLB role references * Improve metallb_speaker_enabled default values
…kubernetes-sigs#9248) * Avoid MetalLB speaker image download when metallb_speaker_enabled is set to * Move metallb_speaker_enabled var to allow outside metalLB role references * Move metallb_speaker_enabled var to allow outside metalLB role references * Improve metallb_speaker_enabled default values
…kubernetes-sigs#9248) * Avoid MetalLB speaker image download when metallb_speaker_enabled is set to * Move metallb_speaker_enabled var to allow outside metalLB role references * Move metallb_speaker_enabled var to allow outside metalLB role references * Improve metallb_speaker_enabled default values
…kubernetes-sigs#9248) * Avoid MetalLB speaker image download when metallb_speaker_enabled is set to * Move metallb_speaker_enabled var to allow outside metalLB role references * Move metallb_speaker_enabled var to allow outside metalLB role references * Improve metallb_speaker_enabled default values
What type of PR is this?
/kind bug
What this PR does / why we need it:
Both MetalLB controller and MetalLB speaker are downloaded when
metallb_enabled: true
since #8715.Since MetalLB speaker can be disabled, its image should be only downloaded when
metallb_speaker_enabled: true