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 tests for semantically equal DaemonSet updates #43406
Add tests for semantically equal DaemonSet updates #43406
Conversation
ed46e26
to
f84ef61
Compare
@k8s-bot bazel test this |
/lgtm |
f84ef61
to
24af8d3
Compare
Rebase because #43337 is merged |
@lavalamp would you approve this change? |
I'm removing the v1.6 milestone since we are lifting code freeze soon and it doesn't seem like we need to hold the branch cut for this. |
/lgtm |
Looks like @lavalamp is the only person who can approve this change. Friendly ping |
@k8s-bot gce etcd3 e2e test this |
@@ -2574,6 +2575,22 @@ run_rs_tests() { | |||
fi | |||
} | |||
|
|||
run_daemonset_tests() { |
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.
Why is this tested here? Shouldn't this be a unit test? Is this to test that kubectl doesn't introduce some sort of change? If so please add a comment.
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.
This is to catch the original issue #43218. Test it here because []
specified in yaml config will be converted to null
.
I have a unit test in validation. Is that not enough?
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'm just wondering if this test adds anything over the unit test you've already put in.
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 see. The validation unit test only tests the validation part. I still want to test that after updating a DaemonSet with a semantically equal change, templateGeneration won't change. It can't be done in DaemonSet controller unit test, because templateGeneration is updated by API server but not DaemonSet controller.
This tests one way it could go wrong, do you have plans for testing other ways? or is this good enough? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: janetkuo, kargakis, lavalamp, liggitt
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
Tests for #43337, depends on #43337. The last commit is already reviewed in #43337.
@liggitt @Kargakis @lukaszo @kubernetes/sig-apps-pr-reviews