-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Revision annotations to conformance tests #600
Add Revision annotations to conformance tests #600
Conversation
In knative/serving#572 Revision was made to be updated with the generation of the Configuration that triggered its creation. This commit updates the conformance tests to assert that it is being populated correctly. Also fixed a bug: after updating the Configuration with a new image, we were (I mean *I was*, I created this bug XD) waiting for the previous Revision to be ready (which of course it was right away), not the new Revision. This means that the health check we had added MIGHT have actually been working (i.e. saving us from 404s and 503s). Snuck in a rename of `routeIsReady` to `isRouteReady` to be consistent with other functions in the file. This partially addresses #593.
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 haven't run the tests yet, but I expect you have :)
/lgtm
/test all |
prow will ;) |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrcunha, bobcatfish, grantr 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 |
…1428) In #475 I removed assertions against Configuration.Spec.Generation b/c it is a hack and not part of the knative spec. In #600 I updated the conformance tests to assert against the Revision annotation which contains the generation. BUT THEN in #778 when I completely re-wrote the tests to no longer use Ginkgo, I accidentally undid both of those changes, so this commit puts them back 😅. BONUS: I hit a case where the length of the loadbalancer ingresses was 0 and got a panic, so if that happens again we'll get an informative error instead.
@bobcatfish: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
In #572 Revision was made to be
updated with the generation of the Configuration that triggered its
creation. This commit updates the conformance tests to assert that it is
being populated correctly.
Also fixed a bug: after updating the Configuration with a new image, we
were (I mean I was, I created this bug XD) waiting for the previous
Revision to be ready (which of course it was right away), not the new
Revision. This means that the health check we had added MIGHT have
actually been working (i.e. saving us from 404s and 503s).
Snuck in a rename of
routeIsReady
toisRouteReady
to be consistentwith other functions in the file.
This partially addresses #593.
Proposed Changes
ConfigurationGenerationAnnotationKey
in conformance tests