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
Assert observedGeneration is incremented in Status.Conditions. #1586
Assert observedGeneration is incremented in Status.Conditions. #1586
Conversation
Hi @dprotaso. 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. |
/test pull-gateway-api-test |
@dprotaso: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
Also question: How come there are no existing conformance tests for GatewayClass mutations? |
/ok-to-test |
What would that look like? The conformance tests accept a GatewayClass to run tests against, but I can't think of anything meaningful to actually test on a GatewayClass other than that it is accepted. |
Prior when asserting against conditions the helpers didn't check their observedGeneration. This means the assertions could occur with a stale status.
Until now with ensuring Thus changing the |
b761ea6
to
6873420
Compare
/test all |
/test all |
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 approach LGTM. This could go into v0.6.0, or a v0.6.1 update later if need be.
Related to kubernetes-sigs/gateway-api#1586 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
…4932) Related to kubernetes-sigs/gateway-api#1586 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
This seems good to me, especially with the opt-out for the GatewayClass for now, so I'm going to hit the approve button. I'll leave it to one of the other approvers to give it the tick. /approve |
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.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, shaneutt, youngnick 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 |
Hey @dprotaso, thanks for all your work on this! The tests LGTM, can you squash this PR into a single commit? (See https://kubernetes.io/docs/contribute/new-content/open-a-pr/#squashing-commits for more info). Once that's done, ping any of us for an LGTM and we'll get it merged. |
Weird isn’t your prow configured to squash on merge?
…On Mon, Dec 19, 2022 at 16:37 Rob Scott ***@***.***> wrote:
Hey @dprotaso <https://github.com/dprotaso>, thanks for all your work on
this! The tests LGTM, can you squash this PR into a single commit? (See
https://kubernetes.io/docs/contribute/new-content/open-a-pr/#squashing-commits
for more info). Once that's done, ping any of us for an LGTM and we'll get
it merged.
—
Reply to this email directly, view it on GitHub
<#1586 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAERARXJ4BFYCJWUNZYLXDWODIRZANCNFSM6AAAAAASXLU5OQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, but I think the suggestion here is since this PR is quite large we would like to give you the opportunity to squash into less commits, if not necessarily one commit. That is to say: if you feel it's important for there to be more than one commit to help keep the history more clear and trackable, do a rebase and squash to a history you feel is solid and we can merge that without simply squashing it all into one. |
I have no preference - if you can squash and merge into a single commit
it’ll save me the trouble of rebasing
…On Mon, Dec 19, 2022 at 17:46 Shane Utt ***@***.***> wrote:
Weird isn’t your prow configured to squash on merge?
Yes, but I think the suggestion here is since this PR is quite large we
would like to give you the opportunity to squash into *less* commits, if
not necessarily *one* commit. That is to say: if you feel it's important
for there to be more than one commit to help keep the history more clear
and trackable, do a rebase and squash to a history you feel is solid and we
can merge that *without* simply squashing it all into one.
—
Reply to this email directly, view it on GitHub
<#1586 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAERATFHT4WDGYFSTPLYG3WODQUDANCNFSM6AAAAAASXLU5OQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
/hold cancel |
@dprotaso I rebased to include this PR and I'm getting an error that wasn't present before. Is this an issue in the PR or in the implementation I'm testing?
|
The test waits for all gateways to be The update can fail if the resource is modified after we fetch it but before we perform the update - this is just regular K8s semantics. My intention of the test is that the gateway resource is stable prior to doing the Can you confirm what the exact mutation is? |
…rojectcontour#4932) Related to kubernetes-sigs/gateway-api#1586 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: yy <yang.yang@daocloud.io>
…rojectcontour#4932) Related to kubernetes-sigs/gateway-api#1586 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: yy <yang.yang@daocloud.io>
…rojectcontour#4932) Related to kubernetes-sigs/gateway-api#1586 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
This PR ensures Gateway Resources and their conditions update observedGeneration as described in the GEP.
Current it tests
Which issue(s) this PR fixes:
Fixes: #1584
Does this PR introduce a user-facing change?: