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

Bug 1343681 - Fix tagsChanged logic #9258

Merged
merged 1 commit into from
Jun 15, 2016
Merged

Bug 1343681 - Fix tagsChanged logic #9258

merged 1 commit into from
Jun 15, 2016

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Jun 9, 2016

@smarterclayton you gonna love this one 😜

smarterclayton
smarterclayton reviewed Jun 9, 2016
View reviewed changes
pkg/image/api/helper.go
@@ -615,7 +615,7 @@ func tagsChanged(new, old []TagEvent) (changed bool, deleted bool) {
case len(old) == 0:
return true, false
default:
return new[0] == old[0], false
return new[0] != old[0], false
Copy link
Contributor

Choose a reason for hiding this comment

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

AWESOME

@smarterclayton
Copy link
Contributor

smarterclayton commented Jun 9, 2016 •
edited by soltysh
Loading

LGTM thanks

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@soltysh
Copy link
Contributor Author

soltysh commented Jun 9, 2016

It's failing unit tests though :( digging into it.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 10, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4899/) (Image: devenv-rhel7_4382)

@soltysh
Copy link
Contributor Author

soltysh commented Jun 15, 2016

It looks like old is being modified in tagsChanged in pkg/image/registry/imagestram/strategy.go, looking into it.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6dd1c69

@soltysh
Copy link
Contributor Author

soltysh commented Jun 15, 2016

@smarterclayton I've removed your merge tag, because I'd want your 2nd look. Esp. are you ok with the change in strategy_test.go which copies the existingTagHistory map (in old stream), so that it's not modified along with the new stream.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4899/)

@smarterclayton
Copy link
Contributor

LGTM [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6dd1c69

@openshift-bot openshift-bot merged commit 30c509f into openshift:master Jun 15, 2016
@soltysh soltysh deleted the bug1343681 branch June 16, 2016 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soltysh @smarterclayton @openshift-bot