Skip to content

Fix MergeConfigs() to work with multiple node pools #1059

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix MergeConfigs() to work with multiple node pools #1059

wants to merge 2 commits into from

Conversation

jimdickinson
Copy link

@jimdickinson jimdickinson commented Jul 25, 2025 •
edited
Loading

Description

Fix MergeConfigs() to work with multiple node pools

Issues Resolved

Fixes #1057

Check List

  • Commits are signed per the DCO using --signoff
  • Unittest added for the new/changed functionality and all unit tests are successful
  • Customer-visible features documented
  • No linter warnings (make lint)

If CRDs are changed:

  • CRD YAMLs updated (make manifests) and also copied into the helm chart
  • Changes to CRDs documented

Please refer to the PR guidelines before submitting this pull request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@jimdickinson jimdickinson requested a review from idanl21 as a code owner July 25, 2025 14:10
@prudhvigodithi prudhvigodithi mentioned this pull request Jul 31, 2025
Open
@rootxrishabh
Copy link
Contributor

Hi @jimdickinson, thanks for the PR!

I think what you are trying to do here is to avoid updating r.instance.Spec.General.AdditionalConfig so that it doesn't update the nodepool that gets reconciled next. Correct?

@jimdickinson
Copy link
Author

Hi @jimdickinson, thanks for the PR!

I think what you are trying to do here is to avoid updating r.instance.Spec.General.AdditionalConfig so that it doesn't update the nodepool that gets reconciled next. Correct?

I think that's right.

rootxrishabh
rootxrishabh approved these changes Aug 22, 2025
View reviewed changes
Copy link
Contributor

@rootxrishabh rootxrishabh left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
cc: @prudhvigodithi

@jimdickinson
Copy link
Author

@prudhvigodithi can you approve the PR? it's still blocked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idanl21 idanl21 Awaiting requested review from idanl21 idanl21 is a code owner

@swoehrl-mw swoehrl-mw Awaiting requested review from swoehrl-mw swoehrl-mw is a code owner

@prudhvigodithi prudhvigodithi Awaiting requested review from prudhvigodithi prudhvigodithi is a code owner

@jochenkressin jochenkressin Awaiting requested review from jochenkressin jochenkressin is a code owner

@pchmielnik pchmielnik Awaiting requested review from pchmielnik pchmielnik is a code owner

@salyh salyh Awaiting requested review from salyh salyh is a code owner

1 more reviewer

@rootxrishabh rootxrishabh rootxrishabh approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Engineering Effectiveness Board
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] additionalConfig being used with multiple node pools does not work correctly
2 participants
@jimdickinson @rootxrishabh