Promote image trigger annotation to GA and correct syntax #16226

smarterclayton · 2017-09-08T04:02:46Z

Use the correct JSONPath syntax (this may be something to backport to
3.6.1).

@bparees @mfojtik @soltysh

Use the correct JSONPath syntax (this may be something to backport to 3.6.1).

bparees · 2017-09-08T04:24:45Z

/lgtm

bparees · 2017-09-08T04:25:07Z

/hold
(we're post-devcut)

smarterclayton · 2017-09-08T05:35:18Z

Pointless to use hold like this. On Sep 8, 2017, at 12:25 AM, Ben Parees <notifications@github.com> wrote: /hold (we're post-devcut) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16226 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p3R-xO8zP5x8e-XClyPBBYuzqQR0ks5sgMGmgaJpZM4PQqxw> .

smarterclayton · 2017-09-08T05:36:16Z

Oh, I didn't see you lgtm. We need to talk about some migration implications first - removing label.

bparees · 2017-09-08T05:42:58Z

Oh, I didn't see you lgtm.

yeah i was trying to get fancy.

We need to talk about some migration implications first - removing label.

but it's alpha! :)

mfojtik · 2017-09-08T09:03:38Z

@smarterclayton what happen to existing resources with this annotation? will they stop working? or we don't care because it was alpha?

soltysh

Overall lgtm, but I'd like a clear answer wrt to migration path.

soltysh · 2017-09-08T10:42:51Z

pkg/image/apis/image/v1/trigger/trigger.go

 // container image references changed when an image stream tag is updated. Today, only
 // containers can be specified by fieldPath.
-const TriggerAnnotationKey = "image.alpha.openshift.io/triggers"
+const TriggerAnnotationKey = "image.openshift.io/triggers"


I'll point out here, what @mfojtik pointed in the comments. Are you thinking of existing users? Do you plan some kind of migration/release notes about this change. Why not supporting both of the annotation for a release for a smooth migration and change only after?

My current thinking is how #16225 does it

soltysh · 2017-09-08T10:44:55Z

pkg/image/trigger/annotations/annotations.go

 	}
 	// TODO: potentially make this more flexible, like whitespace
-	if name := strings.TrimSuffix(strings.TrimPrefix(selector, "?(@.name='"), "')"); name != selector {
+	if name := strings.TrimSuffix(strings.TrimPrefix(selector, "?(@.name==\""), "\")"); name != selector {


I start to wonder what is worse, jsonpath or regexp, 🤔

smarterclayton · 2017-09-08T13:36:10Z

Jsonpath is worse because no one ever spec'd it. On Sep 8, 2017, at 6:45 AM, Maciej Szulik <notifications@github.com> wrote: *@soltysh* approved this pull request. Overall lgtm, but I'd like a clear answer wrt to migration path. ------------------------------ In pkg/image/apis/image/v1/trigger/trigger.go <#16226 (comment)>:

@@ -3,7 +3,7 @@ package trigger

// TriggerAnnotationKey is the annotation used on resources to signal they wish to have // container image references changed when an image stream tag is updated. Today, only // containers can be specified by fieldPath. -const TriggerAnnotationKey = "image.alpha.openshift.io/triggers" +const TriggerAnnotationKey = "image.openshift.io/triggers" I'll point out here, what @mfojtik <https://github.com/mfojtik> pointed in the comments. Are you thinking of existing users? Do you plan some kind of migration/release notes about this change. Why not supporting both of the annotation for a release for a smooth migration and change only after? ------------------------------ In pkg/image/trigger/annotations/annotations.go <#16226 (comment)>:

@@ -78,7 +78,7 @@ func findContainerBySelector(spec ometa.PodSpecReferenceMutator, init bool, sele

return spec.GetContainerByIndex(init, i) } // TODO: potentially make this more flexible, like whitespace - if name := strings.TrimSuffix(strings.TrimPrefix(selector, "?(@.name='"), "')"); name != selector { + if name := strings.TrimSuffix(strings.TrimPrefix(selector, "?(@.name==\""), "\")"); name != selector { I start to wonder what is worse, jsonpath or regexp, 🤔 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16226 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_px7RtEXkOyCcly7v6b9QlCSd42JJks5sgRrEgaJpZM4PQqxw> .

smarterclayton · 2017-09-16T00:53:51Z

/test

smarterclayton · 2017-09-16T17:44:12Z

After thinking about this more I don't think we should provide a migration. Alpha is alpha, and this was only in 3.6. The reason why serving certs deserve a migration of some form is that we are using them in deployed code and have been using them since 3.3. This feature has a smaller window, and our general statement of alpha being unsupported and for preview stands.

bparees · 2017-09-16T17:45:06Z

I'm all for anything that sets a precedent like this :)

smarterclayton · 2017-09-16T17:47:04Z

/retest

smarterclayton · 2017-09-16T18:16:54Z

/retest

soltysh · 2017-09-18T13:05:12Z

After thinking about this more I don't think we should provide a migration. Alpha is alpha, and this was only in 3.6. The reason why serving certs deserve a migration of some form is that we are using them in deployed code and have been using them since 3.3. This feature has a smaller window, and our general statement of alpha being unsupported and for preview stands.

How much more work is to provide the migration. I like precedents (sometimes), but at the same time I'd like to be nice (at least try) for our users and provide the with tooling for migration.

smarterclayton · 2017-09-18T13:27:01Z

Considering this landed in 3.6 (out for a few weeks) and that we are considering backporting the fix, it's hard to say that any amount of work is worth it. If we're going to invest the effort, there are better places to do so. On Sep 18, 2017, at 9:05 AM, Maciej Szulik <notifications@github.com> wrote: After thinking about this more I don't think we should provide a migration. Alpha is alpha, and this was only in 3.6. The reason why serving certs deserve a migration of some form is that we are using them in deployed code and have been using them since 3.3. This feature has a smaller window, and our general statement of alpha being unsupported and for preview stands. How much more work is to provide the migration. I like precedents (sometimes), but at the same time I'd like to be nice (at least try) for our users and provide the with tooling for migration. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16226 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pwPX1jwMXdXCmDpNFv7Upur3dPoYks5sjmqLgaJpZM4PQqxw> .

soltysh · 2017-09-18T13:41:37Z

Considering this landed in 3.6 (out for a few weeks) and that we are considering backporting the fix, it's hard to say that any amount of work is worth it.

If we're going to invest the effort, there are better places to do so.

OK, I think the last sentence convinced me more the the former 😉

soltysh · 2017-09-18T13:42:06Z

/lgtm

openshift-merge-robot · 2017-09-18T13:42:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, smarterclayton, soltysh

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~pkg/image/OWNERS~~ [bparees,smarterclayton,soltysh]
~~pkg/oc/OWNERS~~ [bparees,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-09-18T16:41:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-09-18T20:44:08Z

Automatic merge from submit-queue

Promote image trigger annotation to GA and correct syntax

08c587b

Use the correct JSONPath syntax (this may be something to backport to 3.6.1).

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 8, 2017

openshift-merge-robot assigned soltysh and miminar Sep 8, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2017

openshift-ci-robot assigned bparees Sep 8, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2017

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2017

smarterclayton removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 8, 2017

smarterclayton added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2017

soltysh approved these changes Sep 8, 2017

View reviewed changes

smarterclayton removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2017

openshift-merge-robot merged commit d6d3f97 into openshift:master Sep 18, 2017

Promote image trigger annotation to GA and correct syntax #16226

Promote image trigger annotation to GA and correct syntax #16226

Uh oh!

Conversation

smarterclayton commented Sep 8, 2017

Uh oh!

bparees commented Sep 8, 2017

Uh oh!

bparees commented Sep 8, 2017

Uh oh!

smarterclayton commented Sep 8, 2017 via email

Uh oh!

smarterclayton commented Sep 8, 2017

Uh oh!

bparees commented Sep 8, 2017

Uh oh!

mfojtik commented Sep 8, 2017

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

soltysh Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

smarterclayton Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

soltysh Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Sep 8, 2017 via email

Uh oh!

smarterclayton commented Sep 16, 2017

Uh oh!

smarterclayton commented Sep 16, 2017

Uh oh!

bparees commented Sep 16, 2017

Uh oh!

smarterclayton commented Sep 16, 2017

Uh oh!

smarterclayton commented Sep 16, 2017

Uh oh!

soltysh commented Sep 18, 2017

Uh oh!

smarterclayton commented Sep 18, 2017 via email

Uh oh!

soltysh commented Sep 18, 2017

Uh oh!

soltysh commented Sep 18, 2017

Uh oh!

openshift-merge-robot commented Sep 18, 2017

Uh oh!

openshift-bot commented Sep 18, 2017

Uh oh!

openshift-merge-robot commented Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants