-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 NodeRegistrationOptions.ImagePullPolicy Support #7772
✨ Add NodeRegistrationOptions.ImagePullPolicy Support #7772
Conversation
Welcome @akshay196! |
Hi @akshay196. 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. |
Hi team,
|
/ok-to-test I'll take a look |
Remaining issues should be:
|
@akshay196 Just fyi. That should be the second part that I described now. (you can generate the conversion with |
I executed |
Yup sounds correct. I never remember if the final Now we have to take a look at the unit tests. Seems there is an issue with the conversion somewhere (I"ll take a look) (btw more context here: https://hackmd.io/aYLvrb_jTXiwg_MnpD804w) |
The final
Thanks for sharing that. I don't completely understand how conversion happening here. I took reference from PR #4905 and did corresponding changes. I may have made mistakes. |
@akshay196 No worries. It's unfortunately quite complex. I added some hints as a comment above. After those changes the tests should be green. |
/test pull-cluster-api-e2e-full-main |
capi-e2e test is failing with this error:
Is it some flaky test? |
/retest |
Probably. Let's see |
Another flake :/ /retest |
/lgtm Let's wait for green e2e tests, but then it would be good if you can squash the commits |
LGTM label has been added. Git tree hash: 4251399300b8a52ca787800fe80774c6a0d4af93
|
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.
one nit, otherwise lgtm for me pending squash
1643f0e
to
7ed1548
Compare
7ed1548
to
7103f32
Compare
Add the `imagePullPolicy` field in the `nodeRegistration` section of `InitConfiguration` and `JoinConfiguration`. This allows the user to specify the image pull policy in KCP and CABPK. The value of this field must be one of `Always`, `IfNotPresent` or `Never`. Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>
7103f32
to
4ca79ad
Compare
PR rebased on main and all commits are squashed. 🚀 |
Thank you very much!! /lgtm |
LGTM label has been added. Git tree hash: 671167c3b3dea77402808730f637f93891e02e66
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
Thank you @sbueringer @fabriziopandini for reviewing. |
What this PR does / why we need it:
Add support for ImagePullPolicy in KCP and CABPK.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #6679