-
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 e2e test for rollback scenario #5702
Add e2e test for rollback scenario #5702
Conversation
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.
@taragu: 0 warnings.
In response to this:
/lint
Part of #5285
Proposed Changes
- Add e2e test for rollback scenario
Release Note
NONE
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.
d1cf521
to
17987e7
Compare
test/crd.go
Outdated
@@ -35,6 +35,7 @@ type ResourceNames struct { | |||
TrafficTarget string | |||
URL *url.URL | |||
Image string | |||
BYOName string |
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.
Rather than passing this everywhere, we can just pass a new option to write the name? We probably even have a function to set this.
test/e2e/rollback_byo_test.go
Outdated
|
||
withTrafficSpecOld := WithInlineRouteSpec(v1alpha1.RouteSpec{ | ||
Traffic: []v1alpha1.TrafficTarget{ | ||
{ |
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.
nit: use {{
for less indentation.
test/e2e/rollback_byo_test.go
Outdated
// Now, rollback to the first RevisionSpec | ||
rollbackSvc := resources.Service.DeepCopy() | ||
rollbackSvc.Spec = v1alpha1.ServiceSpec{ | ||
DeprecatedRelease: &v1alpha1.ReleaseType{ |
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.
When we rollback the Service's spec should be identical to when it was first created.
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 whole point of the test is that we should be able to rollback the whole spec (e.g. GitOps) and things should still work. So the progression should be:
First:
spec:
template:
metadata:
name: foo
spec:
containers:
- image: foo
traffic:
- revisionName: foo
percent: 100
Second:
spec:
template:
metadata:
name: bar
spec:
containers:
- image: bar
traffic:
- revisionName: bar
percent: 100
Third:
spec:
template:
metadata:
name: foo
spec:
containers:
- image: foo
traffic:
- revisionName: foo
percent: 100
However, what you have (ignoring release mode) is more like:
spec:
template:
metadata:
name: bar
spec:
containers:
- image: bar
traffic:
- revisionName: foo
percent: 100
The whole point is that we should be able to achieve the third state by simply re-applying the initial state.
05e5536
to
6ac7fe0
Compare
6ac7fe0
to
3a5dfe7
Compare
This PR is ready for another review |
88a0fe6
to
26f18bc
Compare
@dgerd thanks for the review! This PR is ready for another look. |
/lgtm Thanks Tara! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgerd, taragu 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 |
/test pull-knative-serving-unit-tests |
/test pull-knative-serving-unit-tests |
2 similar comments
/test pull-knative-serving-unit-tests |
/test pull-knative-serving-unit-tests |
The following jobs failed:
Automatically retrying due to test flakiness... |
/lint
Part of #5285
Proposed Changes
Release Note