Assert against Revision generation annotation instead of config spec #1428
Conversation
google-prow-robot
added
size/M
|
/lgtm Holding so Jon can review. |
google-prow-robot
added
do-not-merge/hold
|
This will conflict quite a bit with #1420 I also don't love how this conflates readiness with the generation assertions. WDYT about just having two checks? |
bobcatfish
force-pushed
the
no_generation_assertion
branch
from
9e74fd1 to
9cb1a0a
Compare
google-prow-robot
removed
lgtm
|
okay it took me forever but this is finally ready for another look @adrcunha @jonjohnsonjr !
I took your advice @jonjohnsonjr let me know what you think! |
bobcatfish
force-pushed
the
no_generation_assertion
branch
from
9cb1a0a to
896628a
Compare
test/conformance/route_test.go
Outdated
| @@ -95,6 +93,16 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared | |||
| if err != nil { | |||
| t.Fatalf("Revision %s did not become ready to serve traffic: %v", names.Revision, err) | |||
| } | |||
| logger.Infof("The Revision will be annotated with the generation") | |||
| err = test.CheckRevisionState(clients.Revisions, names.Revision, func(r *v1alpha1.Revision) (bool, error) { | |||
| if a, ok := r.Annotations[serving.ConfigurationGenerationAnnotationKey]; ok { | |||
There was a problem hiding this comment.
Pull this logic out into test/states.go?
func RevisionHasGenerationAnnotation(gen int) func(*v1alpha1.Revision) (bool, error) {
// [98,102]
}There was a problem hiding this comment.
ah yes, that's a good idea
In knative/serving#475 I removed assertions against Configuration.Spec.Generation b/c it is a hack and not part of the knative spec. In knative/serving#600 I updated the conformance tests to assert against the Revision annotation which contains the generation. BUT THEN in knative/serving#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
force-pushed
the
no_generation_assertion
branch
from
896628a to
ba0b3ad
Compare
| // IsRevisionAtExpectedGeneration returns a function that will check if the annotations | ||
| // on the revision include an annotation for the generation and that the annotation is | ||
| // set to the expected value. | ||
| func IsRevisionAtExpectedGeneration(expectedGeneration string) func(r *v1alpha1.Revision) (bool, error) { |
There was a problem hiding this comment.
okay so now this is the only function in states.go that returns a function, i think im okay with that but curious what you think @jonjohnsonjr
There was a problem hiding this comment.
See AllRouteTrafficAtRevision
There was a problem hiding this comment.
well nevermind then sir!
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrcunha, bobcatfish 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 |
google-prow-robot
added
the
approved
|
/hold cancel /meow space |
google-prow-robot
removed
the
do-not-merge/hold
|
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. |
|
/test pull-knative-serving-integration-tests |
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.
Proposed Changes