Add one more case to cover @latest as part of the traffic split.#3104
Add one more case to cover @latest as part of the traffic split.#3104knative-prow-robot merged 14 commits intoknative:masterfrom
Conversation
Followup to the main implementation of knative#2819. - fix the test comment, to reflect the reality - add a case where candidate is replaced with @latest - traffic distribution is validated.
knative-prow-robot
added
the
size/S
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@vagababov: 0 warnings.
Details
In response to this:
Followup to the main implementation of #2819.
Proposed Changes
- fix the test comment, to reflect the reality
- add a case where candidate is replaced with @latest
- traffic distribution is validated.
/lint
/cc @dgerd
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.
|
|
||
| // Now update the service to use `@latest` as candidate. | ||
| revisions[1] = v1alpha1.ReleaseLatestRevisionKeyword | ||
| logger.Info("Updating Service to split traffic between two `current` and `@latest`") |
There was a problem hiding this comment.
Your combining of traffic target names with revision keywords confuses me.
| logger.Info("Updating Service to split traffic between two `current` and `@latest`") | |
| logger.Info("Updating Service to split traffic between the second revision and `@latest`") |
| // The domains should not change, since configuration was not changed. | ||
| validateDomains(t, logger, clients, | ||
| names.Domain, | ||
| []string{expectedFirstRev, expectedSecondRev}, |
There was a problem hiding this comment.
I don't think this is right...
Your traffic should be split between the first and third revision. This is checking that the traffic is split between the first and second revision.
I think this is passing because the validation happens faster than our traffic adjustment.
|
/test pull-knative-serving-integration-tests |
|
/test pull-knative-serving-unit-tests |
|
/hold |
knative-prow-robot
added
the
do-not-merge/hold
|
Yeah, we need to wait for the desired shape. Otherwise the test is too flakey. |
knative-prow-robot
added
size/L
|
/retest |
| revisions = append(revisions, names.Revision) | ||
|
|
||
| // Also verify traffic is in the correct shape. | ||
| desiredTrafficShape["latest"] = v1alpha1.TrafficTarget{ |
There was a problem hiding this comment.
For a different PR, but what do you think about making the latest, candidate, and current strings constants in service_types.go since they are part of the API spec.
There was a problem hiding this comment.
Sounds like a good idea. But this would change probably hundreds of files, so let's do it separately.
Co-Authored-By: vagababov <vagababov@users.noreply.github.com>
Co-Authored-By: vagababov <vagababov@users.noreply.github.com>
…itests o explain why this merge is necessary,
|
/retest |
|
/hold cancel |
knative-prow-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
/hold cancel |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, vagababov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
/test pull-knative-serving-integration-tests |
4 participants
Followup to the main implementation of #2819.
Proposed Changes
/lint
/cc @dgerd