-
Notifications
You must be signed in to change notification settings - Fork 122
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
Fix edge-case where scale-from-zero of MCD is mistaken for a rolling update #956
Conversation
Thank you @thiyyakat for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
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.
I would request to add a small unit test around FilterActiveMachineSets
which tests this edge-case
41a8684
to
d9bf45a
Compare
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.
A nit else it looks good
@@ -674,8 +674,16 @@ func (dc *controller) isScalingEvent(ctx context.Context, d *v1alpha1.MachineDep | |||
if err != nil { | |||
return false, err | |||
} | |||
if newIS == nil { |
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 doc string for this method is not in sync with the implementation. Can you please update 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.
/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.
/lgtm
What this PR does / why we need it:
The PR adds a check to distinguish between a rolling update and a scale-from-zero scenario. Additionally, it also updates the integration tests to verify MCM's behaviour for the scenario.
Which issue(s) this PR fixes:
Fixes #952
Special notes for your reviewer:
Integration tests with mcm-provider-gcp passed successfully.
Release note: