Plumb revision annotations through to services and deployments by bsnchan · Pull Request #576 · knative/serving

bsnchan · 2018-04-03T18:40:52Z

Signed-off-by: Brenda Chan brchan@pivotal.io

Fixes Issue #359

Proposed Changes

Only propagate the annotations specified under a
configuration's spec.RevisionTemplate.metadata.annotations
Adds test to ensure revisionTemplate annotations are passed to revisions

* Only propagate the annotations specified under a configuration's spec.RevisionTemplate.metadata.annotations * Adds test to ensure revisionTemplate annotations are passed to revisions knative/serving#359 Signed-off-by: Brenda Chan <brchan@pivotal.io>

googlebot · 2018-04-03T18:40:56Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dprotaso · 2018-04-03T19:01:34Z

I'm always ok!

vaikas · 2018-04-03T20:24:12Z

+			Name:        controller.GetRevisionAutoscalerName(u),
+			Namespace:   AutoscalerNamespace,
+			Labels:      MakeElaResourceLabels(u),
+			Annotations: MakeElaResourceAnnotations(u),


This pulls in the istio sidecar annotation as well, is that something that we want? Are there any side effects on this?

This pulls in the istio sidecar annotation as well

Only if a RevisionTemplate's metadata has specified it. Is that your concern?

Yeah, I was just curious if this is what we want and if the istio sidecar goes in there, will it impact anything?

Will do some digging to confirm

We confirmed that if the RevisionTemplate has an Istio sidecar annotation on the service, it would not impact anything since the annotation only applies to the Pods.

See the istio webhook config for evidence

mdemirhan · 2018-04-03T23:15:18Z

-			Labels:    MakeElaResourceLabels(u),
+			Name:        controller.GetRevisionAutoscalerName(u),
+			Namespace:   AutoscalerNamespace,
+			Labels:      MakeElaResourceLabels(u),


Should we apply the custom annotations and labels to autoscaler pod or should we skip this part? autoscaler is a system component and it might be better to keep it hidden from the user and not copy labels and annotations that the user specified to it. What do you think?

We chose to plumb the labels and annotations through to the autoscaler as well because we thought the intent was that the user can then target all services and pods that are created as part of creating a Configuration.

bsnchan · 2018-04-06T16:21:35Z

@mdemirhan @vaikas-google Let us know what you think of the comments above and if you feel strongly about any of them.

mdemirhan · 2018-04-06T16:39:45Z

I am fine with autoscaler pod having the labels as well.
/lgtm

vaikas · 2018-04-06T19:54:12Z

/lgtm

google-prow-robot · 2018-04-06T19:54:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsnchan, mdemirhan, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas-google]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vaikas · 2018-04-06T19:56:55Z

Hey @dprotaso that "I'm always ok!" comment was giving your approval for your commits getting merged? Just checking before hitting the approve button :)

dprotaso · 2018-04-06T19:57:37Z

Yes :)

…

On Fri, Apr 6, 2018 at 15:56 Ville Aikas ***@***.***> wrote: Hey @dprotaso <https://github.com/dprotaso> that "I'm always ok!" comment was giving your approval for your commits getting merged? Just checking before hitting the approve button :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#576 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIglNI1Ofgqqh0ap51qSX6d0r9SW1Vks5tl8iIgaJpZM4TFlKn> .

mattmoor · 2018-04-11T14:35:37Z

I believe this is completely signed off, but probably won't auto-merge due to the CLA issues (cc @adrcunha ).

I'm going to merge this now as @dprotaso has confirmed things the hard way :)

adrcunha · 2018-04-11T16:13:42Z

I believe this is completely signed off, but probably won't auto-merge due to the CLA issues

The PR was in the merge pool, but it vanished since this was manually merged.

@krzyzacy what would be the right command/approach here? The help page doesn't show any explicit command for "author consent", and I'm not sure if /skip, /approval or /ok-to-test would help here.

krzyzacy · 2018-04-11T17:12:15Z

looks like cla need to be re-signed? cc @cjwagner

google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 3, 2018

vaikas reviewed Apr 3, 2018

View reviewed changes

mdemirhan reviewed Apr 3, 2018

View reviewed changes

google-prow-robot assigned mdemirhan Apr 6, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018

google-prow-robot assigned vaikas Apr 6, 2018

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2018

vaikas approved these changes Apr 6, 2018

View reviewed changes

mattmoor merged commit 715578f into knative:master Apr 11, 2018

tcnghia mentioned this pull request Oct 19, 2018

Add mdemirhan to /OWNERS #2264

Merged

nak3 added a commit to nak3/serving that referenced this pull request Oct 29, 2020

Use CSV with new bundle format (knative#576)

ff40bed

Conversation

bsnchan commented Apr 3, 2018

Proposed Changes

Uh oh!

googlebot commented Apr 3, 2018

Uh oh!

dprotaso commented Apr 3, 2018

Uh oh!

vaikas Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

dprotaso Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

vaikas Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

dprotaso Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

bsnchan Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

mdemirhan Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

bsnchan Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

bsnchan commented Apr 6, 2018

Uh oh!

mdemirhan commented Apr 6, 2018

Uh oh!

vaikas commented Apr 6, 2018

Uh oh!

google-prow-robot commented Apr 6, 2018

Uh oh!

vaikas commented Apr 6, 2018

Uh oh!

dprotaso commented Apr 6, 2018 via email

Uh oh!

mattmoor commented Apr 11, 2018

Uh oh!

adrcunha commented Apr 11, 2018

Uh oh!

krzyzacy commented Apr 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants