Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix annotation trigger to reconcile on container image change #18513

Merged
merged 3 commits into from
Feb 9, 2018
Merged

Fix annotation trigger to reconcile on container image change #18513

merged 3 commits into from
Feb 9, 2018

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented Feb 7, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1541685

@smarterclayton this should fix your issues with DS and annotation trigger

@tnozicka tnozicka added kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Feb 7, 2018
@tnozicka tnozicka added this to the 3.9.0 milestone Feb 7, 2018
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 7, 2018
@tnozicka
Copy link
Contributor Author

tnozicka commented Feb 7, 2018

/cherrypick release-3.8

@openshift-cherrypick-robot
Copy link

@tnozicka: once the present PR merges, I will cherry-pick it on top of release-3.8 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.8

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.

@tnozicka
Copy link
Contributor Author

tnozicka commented Feb 7, 2018

/cherrypick release-3.7

@openshift-cherrypick-robot
Copy link

@tnozicka: once the present PR merges, I will cherry-pick it on top of release-3.7 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.7

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.

@tnozicka
Copy link
Contributor Author

tnozicka commented Feb 7, 2018

@openshift-ci-robot why would you request 2 more people when I've already requested review from 3 people?

@Kargakis ? (you are the bot expert)

0xmichalis
0xmichalis reviewed Feb 7, 2018
View reviewed changes
pkg/image/OWNERS
@@ -11,3 +11,4 @@ approvers:
- soltysh
- mfojtik
- liggitt
- tnozicka
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add yourself as a reviewer too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though approver is a superset includung reviewer. I think I've already done it somewhere and lgtm works there. What's the difference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/gtm works everywhere - the reviewers section is used by the bot to request reviews automatically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now you will have to do image api reviews \o/ :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no, I am going to fall back

@0xmichalis
Copy link
Contributor

@openshift-ci-robot why would you request 2 more people when I've already requested review from 3 people?

Unfortunately, the bot does not identify manual review requests today. If you used the bot's review request command (/cc mfojtik ironcladlou smarterclayton) in your starting post, it would have requested reviews only from the folks you wanted. Please open an issue in k8s.io/test-infra, we need to investigate whether it's possible to avoid automated review requests in case somebody has been manually requested to review.

@tnozicka
Copy link
Contributor Author

tnozicka commented Feb 8, 2018

@Kargakis thanks for the explanation, I am fine using /cc from now on to avoid it. Don't want to add you more work.

@tnozicka tnozicka force-pushed the fix-annotation-trigger-reconciliation branch from 7a9f71a to 7057684 Compare February 8, 2018 08:12
mfojtik
mfojtik reviewed Feb 8, 2018
View reviewed changes
pkg/image/trigger/annotations/annotations.go

newContainer, _, err := ContainerForObjectFieldPath(newObj, trigger.FieldPath)
if err != nil {
glog.V(5).Infof("%v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add context for this error

mfojtik
mfojtik reviewed Feb 8, 2018
View reviewed changes
pkg/image/trigger/annotations/annotations.go

oldContainer, _, err := ContainerForObjectFieldPath(oldObj, trigger.FieldPath)
if err != nil {
// might just be a result of the update
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glog(5) here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the triggers has changed or the images and the field path is valid only for the new image+trigger combination there will always be an error here although it's not an error in this case

@mfojtik
Copy link
Contributor

mfojtik commented Feb 8, 2018

LGTM, will tag on green tests

@mfojtik
Copy link
Contributor

mfojtik commented Feb 8, 2018

/retest

@mfojtik
Copy link
Contributor

mfojtik commented Feb 8, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, tnozicka

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

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

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2018
@0xmichalis 0xmichalis mentioned this pull request Feb 8, 2018
Closed
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515).

@openshift-merge-robot openshift-merge-robot merged commit afa6a25 into openshift:master Feb 9, 2018
@openshift-cherrypick-robot
Copy link

@tnozicka: #18513 failed to apply on top of branch "release-3.8":

Applying: Add tests for annotation trigger reconciliation
Applying: Add tnozicka to pkg/image approvers since we own part of that code
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/image/OWNERS
Falling back to patching base and 3-way merge...
Auto-merging pkg/image/OWNERS
CONFLICT (content): Merge conflict in pkg/image/OWNERS
Patch failed at 0003 Add tnozicka to pkg/image approvers since we own part of that code

In response to this:

/cherrypick release-3.8

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.

@openshift-cherrypick-robot
Copy link

@tnozicka: #18513 failed to apply on top of branch "release-3.7":

Applying: Add tests for annotation trigger reconciliation
Applying: Add tnozicka to pkg/image approvers since we own part of that code
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/image/OWNERS
Falling back to patching base and 3-way merge...
Auto-merging pkg/image/OWNERS
CONFLICT (content): Merge conflict in pkg/image/OWNERS
Patch failed at 0003 Add tnozicka to pkg/image approvers since we own part of that code

In response to this:

/cherrypick release-3.7

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.

@tnozicka tnozicka deleted the fix-annotation-trigger-reconciliation branch February 9, 2018 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

@0xmichalis 0xmichalis 0xmichalis left review comments

@ironcladlou ironcladlou Awaiting requested review from ironcladlou

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@bparees bparees Awaiting requested review from bparees

Assignees

@mfojtik mfojtik

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. priority/P1 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v3.9
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tnozicka @openshift-cherrypick-robot @0xmichalis @mfojtik @openshift-ci-robot @openshift-merge-robot