By default, set `Image` to the image of the prev. revision by digest #373

sixolet · 2019-08-16T22:30:37Z

Fixes #280

Proposed Changes

By default, when the user does not request an image change, the running code should not change.

Set the Image on a non-image service update to the image by digest of the previous revision
Add a --no-lock-to-digest flag to disable this behavior (for CI systems &c)

Implement setting Image
Add user image annotation
Display user image annotation as image in describe and list
Tests for service update
Tests for user image annotation display
Unit tests

Release Note

knative-prow-robot

@sixolet: 8 warnings.

In response to this:

Fixes #280

Proposed Changes

By default, when the user does not request an image change, the running code should not change.

Set the Image on a non-image service update to the image by digest of the previous revision

Add a --no-lock-to-digest flag to disable this behavior (for CI systems &c)

Implement setting Image

Add user image annotation

Display user image annotation as image in describe and list

Tests for service update

Tests for user image annotation display

Unit tests

Release Note

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.

pkg/kn/commands/service/service_update_test.go

knative-prow-robot · 2019-08-16T22:30:47Z

pkg/serving/config_changes.go

@@ -128,6 +128,21 @@ func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) e
 	return nil
 }

+func FreezeImageToDigest(template *servingv1alpha1.RevisionTemplateSpec, baseRevision *servingv1alpha1.Revision) error {


Golint comments: exported function FreezeImageToDigest should have comment or be unexported. More info.

knative-prow-robot · 2019-08-16T22:30:47Z

pkg/serving/config_changes.go

+		return err
+	}
+	if currentContainer.Image != baseContainer.Image {
+		return fmt.Errorf("Could not freeze image to digest since current revision contains unexpected image.")


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

knative-prow-robot · 2019-08-16T22:30:47Z

pkg/serving/v1alpha1/client.go

@@ -230,6 +240,62 @@ func (cl *knClient) GetRevision(name string) (*v1alpha1.Revision, error) {
 	return revision, nil
 }

+type NoBaseRevisionError struct {


Golint comments: exported type NoBaseRevisionError should have comment or be unexported. More info.

knative-prow-robot · 2019-08-16T22:30:48Z

pkg/serving/v1alpha1/client_mock.go

@@ -194,6 +194,10 @@ func (r *Recorder) GetConfiguration(name string, config *v1alpha1.Configuration,

 }

+func (cl *MockKnClient) GetBaseRevision(service *v1alpha1.Service) (*v1alpha1.Revision, error) {


Golint naming: receiver name cl should be consistent with previous receiver name c for MockKnClient. More info.

googlebot · 2019-08-16T22:32:28Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-16T22:32:29Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-16T22:46:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

duglin · 2019-08-17T02:16:40Z

if we do this can we PLEASE make the default to not change the user's spec on them? I think that should require an explicit request. I don't think it's appropriate to modify the user's input by default.

sixolet · 2019-08-17T05:04:54Z

The alternative is modifying the user's running code. The spec is harmless and cosmetic compared to that.

…

On Aug 16, 2019, at 7:16 PM, Doug Davis ***@***.***> wrote: if we do this can we PLEASE make the default to not change the user's spec on them? I think that should require an explicit request. I don't think it's appropriate to modify the user's input by default. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

duglin · 2019-08-19T13:29:30Z

The alternative is modifying the user's running code. The spec is harmless and cosmetic compared to that.

Not sure about that since that's what the user asked for.

Right now there's already a flag that allows people to control whether their image is "locked down" or not - it's the "image" property. If they provide a static "tag" or a digest then they're asking for things to be locked down. If they provide a dynamic "tag" then they're not. The introduction of this flag now adds a third variant to this boolean choice - meaning, the previous "do not lock down" is now split into "implied lock down" and "not locked down but only if they specify --yes-I-really-want-this on each and every service update. We're forcing people to jump through hoops unnecessary.

I'd like to step back and look at the higher-level feature request because I don't think it's a kn specific issue. I've heard people talk about how the KnService model is a delta model - meaning people should not be required to provide the entire new version of the Service they want to deploy next, rather just the small deltas. If this is really the case then this should apply regardless of the CLI being used - meaning it should be true even when using kubectl. My point is that this isn't a CLI issue, rather it's a server issue.

If we believe that updating the env vars should not cause us to re-get the digest then that should be true for all CLIs and this should be solved at the server/API level so we have a consistent UX no matter how people talk to KnServing. Some folks have mentioned that this is a challenge due to there not being a good way for a user to "poke" the re-getting of the digest but there are ways to do this, we've even talked about some, but to toss up our hands and force a --really-do-what-I-asked-for flag on the user is not a good alternative.

Also, modifying the user provided spec data (w/o them asking) is bad form in Kubernertes. Yes there are some examples where this happens today, but I believe those are rare. So, the act of converting an image name:tag into a digest should be an explicit request from the user and I think it would be better to be done on the initial specification of the non-digest image name rather than during a subsequent request - which could be very surprising if someone thinks they're just update an env var and end's up modifying some other part of the spec. So, a --to-digest (or --lock-image) flag would give the user the ability to explicitly ask for us to do this mapping on their behalf.

Finally, regardless of which way things are defaulted (if we do this at all), it would be nice if this were set just once on a per-service basis. If people like this "map to digest for me" feature, but they don't want it for all of their services (meaning a config file option isn't a good choice for them) then asking them to set it on all interactions with the service can be quite tedious and risky since they may forget which service needs this auto-mapping and which do not. It would be better if they could set a flag once per service (perhaps as an annotation) and then the mapping would happen (or not) for them on that service forever.

This would also allow us to implement this on the server side and all CLIs would get the benefit.

sixolet · 2019-08-19T14:38:14Z

There have been literally hours of conversation about this in the serving working group, and "the client should set the image to the digest" was our conclusion. Having the moral equivalent of an image pull policy was too confusing, having a flag you have to set and remember to unset the next time that told you to pull the image was too tricky. I promise you we considered alternatives.

…

On Aug 19, 2019, at 6:29 AM, Doug Davis ***@***.***> wrote: The alternative is modifying the user's running code. The spec is harmless and cosmetic compared to that. Not sure about that since that's what the user asked for. Right now there's already a flag that allows people to control whether their image is "locked down" or not - it's the "image" property. If they provide a static "tag" or a digest then they're asking for things to be locked down. If they provide a dynamic "tag" then they're not. The introduction of this flag now adds a third variant to this boolean choice - meaning, the previous "do not lock down" is now split into "implied lock down" and "not locked down but only if they specify --yes-I-really-want-this on each and every service update. We're forcing people to jump through hoops unnecessary. I'd like to step back and look at the higher-level feature request because I don't think it's a kn specific issue. I've heard people talk about how the KnService model is a delta model - meaning people should not be required to provide the entire new version of the Service they want to deploy next, rather just the small deltas. If this is really the case then this should apply regardless of the CLI being used - meaning it should be true even when using kubectl. My point is that this isn't a CLI issue, rather it's a server issue. If we believe that updating the env vars should not cause us to re-get the digest then that should be true for all CLIs and this should be solved at the server/API level so we have a consistent UX no matter how people talk to KnServing. Some folks have mentioned that this is a challenge due to there not being a good way for a user to "poke" the re-getting of the digest but there are ways to do this, we've even talked about some, but to toss up our hands and force a --really-do-what-I-asked-for flag on the user is not a good alternative. Also, modifying the user provided spec data (w/o them asking) is bad form in Kubernertes. Yes there are some examples where this happens today, but I believe those are rare. So, the act of converting an image name:tag into a digest should be an explicit request from the user and I think it would be better to be done on the initial specification of the non-digest image name rather than during a subsequent request - which could be very surprising if someone thinks they're just update an env var and end's up modifying some other part of the spec. So, a --to-digest (or --lock-image) flag would give the user the ability to explicitly ask for us to do this mapping on their behalf. Finally, regardless of which way things are defaulted (if we do this at all), it would be nice if this were set just once on a per-service basis. If people like this "map to digest for me" feature, but they don't want it for all of their services (meaning a config file option isn't a good choice for them) then asking them to set it on all interactions with the service can be quite tedious and risky since they may forget which service needs this auto-mapping and which do not. It would be better if they could set a flag once per service (perhaps as an annotation) and then the mapping would happen (or not) for them on that service forever. This would also allow us to implement this on the server side and all CLIs would get the benefit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sixolet · 2019-08-19T15:03:41Z

I agree with "set this behavior once", but it's more on a per-org basis. If you never mutate tags, you don't need it. So let's put it into the kn config file — set it how your org wants, move on.

…

On Aug 19, 2019, at 6:29 AM, Doug Davis ***@***.***> wrote: The alternative is modifying the user's running code. The spec is harmless and cosmetic compared to that. Not sure about that since that's what the user asked for. Right now there's already a flag that allows people to control whether their image is "locked down" or not - it's the "image" property. If they provide a static "tag" or a digest then they're asking for things to be locked down. If they provide a dynamic "tag" then they're not. The introduction of this flag now adds a third variant to this boolean choice - meaning, the previous "do not lock down" is now split into "implied lock down" and "not locked down but only if they specify --yes-I-really-want-this on each and every service update. We're forcing people to jump through hoops unnecessary. I'd like to step back and look at the higher-level feature request because I don't think it's a kn specific issue. I've heard people talk about how the KnService model is a delta model - meaning people should not be required to provide the entire new version of the Service they want to deploy next, rather just the small deltas. If this is really the case then this should apply regardless of the CLI being used - meaning it should be true even when using kubectl. My point is that this isn't a CLI issue, rather it's a server issue. If we believe that updating the env vars should not cause us to re-get the digest then that should be true for all CLIs and this should be solved at the server/API level so we have a consistent UX no matter how people talk to KnServing. Some folks have mentioned that this is a challenge due to there not being a good way for a user to "poke" the re-getting of the digest but there are ways to do this, we've even talked about some, but to toss up our hands and force a --really-do-what-I-asked-for flag on the user is not a good alternative. Also, modifying the user provided spec data (w/o them asking) is bad form in Kubernertes. Yes there are some examples where this happens today, but I believe those are rare. So, the act of converting an image name:tag into a digest should be an explicit request from the user and I think it would be better to be done on the initial specification of the non-digest image name rather than during a subsequent request - which could be very surprising if someone thinks they're just update an env var and end's up modifying some other part of the spec. So, a --to-digest (or --lock-image) flag would give the user the ability to explicitly ask for us to do this mapping on their behalf. Finally, regardless of which way things are defaulted (if we do this at all), it would be nice if this were set just once on a per-service basis. If people like this "map to digest for me" feature, but they don't want it for all of their services (meaning a config file option isn't a good choice for them) then asking them to set it on all interactions with the service can be quite tedious and risky since they may forget which service needs this auto-mapping and which do not. It would be better if they could set a flag once per service (perhaps as an annotation) and then the mapping would happen (or not) for them on that service forever. This would also allow us to implement this on the server side and all CLIs would get the benefit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sixolet · 2019-08-19T18:00:28Z

/restest

duglin · 2019-08-19T19:03:57Z

but it's more on a per-org basis.

I'm sure I'm not alone in this... I don't have a single org I work with. Nor will there always be a single CLI/user per service. So you run the risk of different users applying (or not) this setting to the same service and getting it wrong. This type of setting really needs to be stored with the service itself so it can be applied consistently regardless of who might tweak it.

evankanderson · 2019-08-19T20:43:24Z

Is this a setting which might be best applied on a per-namespace basis?

…

On Mon, Aug 19, 2019 at 12:03 PM Doug Davis ***@***.***> wrote: but it's more on a per-org basis. I'm sure I'm not alone in this... I don't have a single org I work with. Nor will there always be a single CLI/user per service. So you run the risk of different users applying (or not) this setting to the same service and getting it wrong. This type of setting really needs to be stored with the service itself so it can be applied consistently regardless of who might tweak it. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#373?email_source=notifications&email_token=AB4XEN4SWADSIUMKOJSDOI3QFLVB7A5CNFSM4IMNJAQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4T7JRY#issuecomment-522712263>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4XENZ6QXVCDKA4NDHNKN3QFLVB7ANCNFSM4IMNJAQA> .

-- Evan Anderson <[email protected]>

sixolet · 2019-08-19T21:09:28Z

Yeah namespace does sound like a good scope. Is a Namespace object kosher to query for a label though?

…

On Aug 19, 2019, at 1:43 PM, Evan Anderson ***@***.***> wrote: Is this a setting which might be best applied on a per-namespace basis? On Mon, Aug 19, 2019 at 12:03 PM Doug Davis ***@***.***> wrote: > but it's more on a per-org basis. > > I'm sure I'm not alone in this... I don't have a single org I work with. > Nor will there always be a single CLI/user per service. So you run the risk > of different users applying (or not) this setting to the same service and > getting it wrong. This type of setting really needs to be stored with the > service itself so it can be applied consistently regardless of who might > tweak it. > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#373?email_source=notifications&email_token=AB4XEN4SWADSIUMKOJSDOI3QFLVB7A5CNFSM4IMNJAQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4T7JRY#issuecomment-522712263>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AB4XENZ6QXVCDKA4NDHNKN3QFLVB7ANCNFSM4IMNJAQA> > . > -- Evan Anderson ***@***.***> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

evankanderson · 2019-08-19T22:52:03Z

Talking with various people in Seattle (Dan, Matt, and Neil), I think it might be most natural to be able to store this information as an annotation on the Service object -- possibly even using the client.knative.dev/user-image as the hint as to whether the label should be continuously re-resolved or not. This also allows initial deploy to set a policy for future deploys, e.g.

kn create --lock-to-digest docker.io/awesome/i-am-awesome:latest
# Creates the Service with client.knative.dev/user-image annotation set to ":latest"
kn deploy --no-lock-to-digest docker.io/awesome/i-am-awesome:latest
# Removes the client.knative.dev/user-image annotation, causing future deploys to "float" forward

sixolet · 2019-08-19T23:12:27Z

That's a pretty good compromise. Now we just have to pick a default for service creation.

…

On Aug 19, 2019, at 3:52 PM, Evan Anderson ***@***.***> wrote: Talking with various people in Seattle (Dan, Matt, and Neil), I think it might be most natural to be able to store this information as an annotation on the Service object -- possibly even using the client.knative.dev/user-image as the hint as to whether the label should be continuously re-resolved or not. This also allows initial deploy to set a policy for future deploys, e.g. kn create --lock-to-digest docker.io/awesome/i-am-awesome:latest # Creates the Service with client.knative.dev/user-image annotation set to ":latest" kn deploy --no-lock-to-digest docker.io/awesome/i-am-awesome:latest # Removes the client.knative.dev/user-image annotation, causing future deploys to "float" forward — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

duglin · 2019-08-20T13:34:20Z

Now we just have to pick a default for service creation

You know how I feel :-) do what the user asked for - meaning if they gave a floating tag then stick with a floating tag. If they want us to twiddle their input data then they should ask for it.

knative-metrics-robot · 2019-08-22T23:29:53Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	82.8%	83.7%	1.0
pkg/kn/commands/service/service_describe.go	85.2%	85.5%	0.3
pkg/kn/commands/service/service_update.go	71.4%	72.2%	0.8
pkg/kn/commands/testing_helper.go	97.4%	95.0%	-2.4
pkg/kn/flags/bool.go	86.5%	87.2%	0.7
pkg/serving/config_changes.go	80.7%	80.7%	0.1
pkg/serving/v1alpha1/client.go	84.1%	82.8%	-1.2
pkg/serving/v1alpha1/client_mock.go	96.8%	95.8%	-1.0

jimcurtis

/lgtm

knative-prow-robot · 2019-08-27T17:44:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimcurtis, sixolet

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:

~~OWNERS~~ [sixolet]

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

Bonus: linted dep-collector's `README.md`.

sixolet added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 16, 2019

knative-prow-robot requested review from evankanderson and navidshaikh August 16, 2019 22:30

knative-prow-robot reviewed Aug 16, 2019

View reviewed changes

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 16, 2019

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Aug 16, 2019

sixolet force-pushed the by-digest branch from 854861c to 4506014 Compare August 16, 2019 22:46

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Aug 16, 2019

sixolet added 2 commits August 20, 2019 14:07

Option to freeze revision to digest

0594710

Tests pass. Checkpoint.

1f58899

sixolet added 4 commits August 20, 2019 14:07

Tests for env now check pinning to digest

dc9538a

Bool flag using convention. Tweak usage.

ef41602

Describing the image carefully using the annotation and digest

f32ddd3

lint

fc03daf

sixolet force-pushed the by-digest branch from 5182be9 to fc03daf Compare August 20, 2019 21:07

Test matrix of locking to digest behaviors

784c658

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2019

sixolet added 8 commits August 21, 2019 16:25

Expose both flags, and rewrite help text again

68e3df4

Unit tests.

766a72c

Removed unsed method

4892f29

Add tests for getting base revision

861d7cf

Make tests actually test stuff better

11e9eb6

Make tests actually test stuff better

3acf48b

A mergeout killed a returning of error. Restore it

81ca3a0

Help text again

2c28e74

jimcurtis approved these changes Aug 27, 2019

View reviewed changes

knative-prow-robot assigned jimcurtis Aug 27, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019

knative-prow-robot merged commit 0ff537a into knative:master Aug 27, 2019

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Don't lint markdown files in /vendor (knative#373)

d48d493

Bonus: linted dep-collector's `README.md`.

navidshaikh pushed a commit to navidshaikh/client that referenced this pull request Jun 17, 2020

Update SPEC file for release v0.14.0 (knative#373)

e18427b

dsimansk added a commit to dsimansk/client that referenced this pull request May 7, 2024

[release-v1.13] Update cloudevents sdk to latest (knative#373)

0d12ce9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

By default, set `Image` to the image of the prev. revision by digest #373

By default, set `Image` to the image of the prev. revision by digest #373

sixolet commented Aug 16, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot Aug 16, 2019

knative-prow-robot Aug 16, 2019

knative-prow-robot Aug 16, 2019

knative-prow-robot Aug 16, 2019

googlebot commented Aug 16, 2019

googlebot commented Aug 16, 2019

googlebot commented Aug 16, 2019

duglin commented Aug 17, 2019

sixolet commented Aug 17, 2019 via email

duglin commented Aug 19, 2019

sixolet commented Aug 19, 2019 via email

sixolet commented Aug 19, 2019 via email

sixolet commented Aug 19, 2019

duglin commented Aug 19, 2019

evankanderson commented Aug 19, 2019 via email

sixolet commented Aug 19, 2019 via email

evankanderson commented Aug 19, 2019

sixolet commented Aug 19, 2019 via email

duglin commented Aug 20, 2019

knative-metrics-robot commented Aug 22, 2019

jimcurtis left a comment

knative-prow-robot commented Aug 27, 2019

		@@ -194,6 +194,10 @@ func (r Recorder) GetConfiguration(name string, config v1alpha1.Configuration,

		}

		func (cl MockKnClient) GetBaseRevision(service v1alpha1.Service) (*v1alpha1.Revision, error) {

By default, set Image to the image of the prev. revision by digest #373

By default, set Image to the image of the prev. revision by digest #373

Conversation

sixolet commented Aug 16, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot Aug 16, 2019

Choose a reason for hiding this comment

knative-prow-robot Aug 16, 2019

Choose a reason for hiding this comment

knative-prow-robot Aug 16, 2019

Choose a reason for hiding this comment

knative-prow-robot Aug 16, 2019

Choose a reason for hiding this comment

googlebot commented Aug 16, 2019

googlebot commented Aug 16, 2019

googlebot commented Aug 16, 2019

duglin commented Aug 17, 2019

sixolet commented Aug 17, 2019 via email

duglin commented Aug 19, 2019

sixolet commented Aug 19, 2019 via email

sixolet commented Aug 19, 2019 via email

sixolet commented Aug 19, 2019

duglin commented Aug 19, 2019

evankanderson commented Aug 19, 2019 via email

sixolet commented Aug 19, 2019 via email

evankanderson commented Aug 19, 2019

sixolet commented Aug 19, 2019 via email

duglin commented Aug 20, 2019

knative-metrics-robot commented Aug 22, 2019

jimcurtis left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 27, 2019

By default, set `Image` to the image of the prev. revision by digest #373

By default, set `Image` to the image of the prev. revision by digest #373

sixolet commented Aug 16, 2019 •

edited

Loading