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

🐛 Ensure status updates succeed #1261

Merged
merged 1 commit into from
Aug 15, 2019
Merged

🐛 Ensure status updates succeed #1261

merged 1 commit into from
Aug 15, 2019

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented Aug 15, 2019

Signed-off-by: Chuck Ha chuckh@vmware.com
What this PR does / why we need it:
This PR is a work around for #1259.

It also removes an outdated hack in the machine-update code that is no longer needed as of controller-runtime beta.5

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
This can be fixed in controller-runtime but until that fix is in this hack will be in place.

Special notes for your reviewer:

Release note:

NONE

/assign @detiber @vincepri

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 15, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2019
detiber
detiber reviewed Aug 15, 2019
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

some minor nits around the comment, otherwise lgtm.

I suspect the MachineSet and MachineDeployment controllers will also need to be updated as well.

controllers/cluster_controller.go Outdated Show resolved Hide resolved
pkg/controller/machine/machine_controller.go Outdated Show resolved Hide resolved
@chuckha chuckha force-pushed the status-workaround branch 2 times, most recently from 29a0e2b to e8e70d9 Compare August 15, 2019 15:10
vincepri
vincepri reviewed Aug 15, 2019
View reviewed changes
controllers/suite_test.go Outdated Show resolved Hide resolved
@vincepri
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, vincepri

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 Aug 15, 2019
detiber
detiber reviewed Aug 15, 2019
View reviewed changes
controllers/cluster_controller_test.go Outdated
@@ -49,4 +50,37 @@ var _ = Describe("Cluster Reconciler", func() {
}, timeout).Should(BeTrue())
})

It("Should patch the status even if the spec is empty", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Could we also add some tests around patchCluster to verify the edge cases there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that message needs improvement.

It does both update status if spec is empty and update spec if status is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's not satisfactory can you please tell me which edge cases you'd like tests for?

Copy link
Member

Choose a reason for hiding this comment

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

I'd split it in 2 parts :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha i just went the opposite route and made the test name very generic. I can do two different tests if you like though

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of the following three scenarios:

  • Only status updated
  • Only non-status updated
  • Both updated
    Tested independently to ensure we see the expected changes reflected in the resulting object.

@chuckha chuckha force-pushed the status-workaround branch 3 times, most recently from ed9fed4 to 7d51b86 Compare August 15, 2019 15:30
@chuckha
Copy link
Contributor Author

chuckha commented Aug 15, 2019

@vincepri updated to two separate tests

@vincepri vincepri mentioned this pull request Aug 15, 2019
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2019
@chuckha
Ensure status updates succeed
f4f51a8 
Signed-off-by: Chuck Ha <chuckh@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2019
@chuckha
Copy link
Contributor Author

chuckha commented Aug 15, 2019

@detiber rebase fixed and all 3 cases are covered by tests. PTAL

detiber
detiber reviewed Aug 15, 2019
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 56bd7d7 into kubernetes-sigs:master Aug 15, 2019
@chuckha chuckha mentioned this pull request Aug 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@detiber detiber detiber left review comments

@vincepri vincepri vincepri left review comments

@ncdc ncdc Awaiting requested review from ncdc

Assignees

@detiber detiber

@vincepri vincepri

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chuckha @vincepri @k8s-ci-robot @detiber