Annotate revision with the configuration generation label#572
Annotate revision with the configuration generation label#572grantr merged 1 commit intoknative:masterfrom johnugeorge:master
Conversation
johnugeorge
requested review from
evankanderson,
mattmoor and
vaikas
as code owners
google-prow-robot
added
the
size/S
|
/assign evankanderson
…On Mon, Apr 2, 2018 at 5:02 PM Johnu George ***@***.***> wrote:
@johnugeorge <https://github.com/johnugeorge> requested your review on:
knative/serving#572 <#572>
Annotate revision with the configuration generation label as a code owner.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#572 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHlyNxBs0K_YBZcsdV1j1gm3eAzfKIF7ks5tkrwpgaJpZM4TEQV1>
.
--
Evan Anderson <argent@google.com>
|
grantr
left a comment
grantr
left a comment
There was a problem hiding this comment.
Looks good @johnugeorge, thanks! @bobcatfish @evankanderson, should this be exercised in conformance tests since it's part of the spec?
|
/retest |
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
I'm not sure there's much value in indexing the configurationGeneration, unlike the configuration and route associated with the revision.
| rev.ObjectMeta.Labels = make(map[string]string) | ||
| } | ||
| rev.ObjectMeta.Labels[ela.ConfigurationLabelKey] = config.Name | ||
| rev.ObjectMeta.Labels[ela.ConfigurationGenerationLabelKey] = fmt.Sprintf("%v", config.Spec.Generation) |
There was a problem hiding this comment.
Should this be a label or an annotation?
Labels can be used to select objects and to find collections of objects that satisfy certain conditions. In contrast, annotations are not used to identify and select objects
|
@evankanderson I am not sure if there were some discussions already which resulted in the spec designed in #458 |
@grantr yeah I think so, and there have been a few other changes recently we should probably include as well. I'll follow up on it. |
|
/lgtm |
google-prow-robot
added
lgtm
google-prow-robot
added
the
do-not-merge/hold
|
/hold cancel Oops, ignore my previous comment. I forgot to check whether this PR had been updated already (it has). |
google-prow-robot
removed
the
do-not-merge/hold
| } | ||
|
|
||
| if rev.Annotations[ela.ConfigurationGenerationAnnotationKey] != fmt.Sprintf("%v", config.Spec.Generation) { | ||
| t.Errorf("rev does not have generation label <%s:%s>", ela.ConfigurationGenerationAnnotationKey, config.Name) |
There was a problem hiding this comment.
This should probably say "generation annotation" instead of "generation label"
Fixes #458 Adds revision label with generation of the configuration that created the revision
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, grantr, johnugeorge 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 |
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` to `isRouteReady` to be consistent with other functions in the file. This partially addresses #593.
5 participants
Fixes #458
Adds a revision label with generation of the configuration that created the revision