Skip to content

remove job from tag #12358

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
Merged
duglin merged 1 commit into from
Apr 19, 2015
Merged

remove job from tag #12358

duglin merged 1 commit into from
Apr 19, 2015

Conversation

@dalanlan
Copy link
Contributor

@dalanlan dalanlan commented Apr 14, 2015

part of #12151
remove job from tag
Signed-off-by: Simei He hesimei@zju.edu.cn

@dalanlan
Copy link
Contributor Author

dalanlan commented Apr 14, 2015

ping @tiborvass
plz kindly have a check :)

@icecrime icecrime mentioned this pull request Apr 15, 2015
Closed
41 tasks
LK4D4
LK4D4 reviewed Apr 15, 2015
View reviewed changes
graph/tag.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 15, 2015

Choose a reason for hiding this comment

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

Can we call it just Tag?

@dalanlan dalanlan force-pushed the remove_job_from_tag branch from 5a86add to 150d8d7 Compare April 17, 2015 07:27
@dalanlan dalanlan force-pushed the remove_job_from_tag branch from 150d8d7 to aa41008 Compare April 17, 2015 07:34
@dalanlan dalanlan force-pushed the remove_job_from_tag branch from aa41008 to 346ba96 Compare April 17, 2015 07:41
@dalanlan
Copy link
Contributor Author

dalanlan commented Apr 18, 2015

@LK4D4 Fixed now:)

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 18, 2015

@dalanlan Is tests already in integration-cli?

@dalanlan
Copy link
Contributor Author

dalanlan commented Apr 18, 2015

Yep @LK4D4
See also Console Output
[PASSED]: tag - busybox -> testfoobarbaz
[PASSED]: tag - busybox's image ID -> testfoobarbaz
[PASSED]: tag - busybox invalid repo names --> must not work
[PASSED]: tag - busybox with invalid repo:tagnames --> must not work
[PASSED]: tag - tag valid prefixed repo
[PASSED]: tag - busybox with an existed tag name without -f option --> must not
[PASSED]: tag - busybox with an existed tag name with -f option work
[PASSED]: tag - tag official names

duglin
duglin reviewed Apr 18, 2015
View reviewed changes
graph/tag.go Outdated
Copy link
Contributor

@duglin duglin Apr 18, 2015

Choose a reason for hiding this comment

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

Is it worth even keeping this file around any more? The only value I see here is the name Tag() instead of Set() - which better aligns with what an end user sees. Perhaps we could remove this file and rename tags.go's Set() to Tag() - since its just a wrapper for SetLoad().
@LK4D4 what do you think?

Copy link
Contributor

@LK4D4 LK4D4 Apr 18, 2015

Choose a reason for hiding this comment

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

Makes sense!

@duglin
Copy link
Contributor

duglin commented Apr 18, 2015

Aside from my one minor comment, it looks good.

@dalanlan
Copy link
Contributor Author

dalanlan commented Apr 19, 2015

@duglin @LK4D4
updated :)

duglin
duglin reviewed Apr 19, 2015
View reviewed changes
integration/server_test.go Outdated
Copy link
Contributor

@duglin duglin Apr 19, 2015

Choose a reason for hiding this comment

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

I see the other 2 tests in integration-cli, but I'm not sure I see this one. Do you see it?

Copy link
Member

@cpuguy83 cpuguy83 Apr 19, 2015

Choose a reason for hiding this comment

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

@duglin It's in there in the containers_api test.

Copy link
Contributor

@duglin duglin Apr 19, 2015

Choose a reason for hiding this comment

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

I'm not seeing it - must be blind. Got a URL?

Copy link
Member

@cpuguy83 cpuguy83 Apr 19, 2015

Choose a reason for hiding this comment

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

Sorry, it's in the run tests now, and there is a PR to move it to API tests...
https://github.com/docker/docker/blob/master/integration-cli/docker_cli_run_test.go#L3500

Copy link
Contributor

@duglin duglin Apr 19, 2015

Choose a reason for hiding this comment

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

ok thanks

@duglin
Copy link
Contributor

duglin commented Apr 19, 2015

Please squash the commits down to one.
Aside from my on question about the test, it looks good.

@dalanlan
remove job from tag
99f6309 
Signed-off-by: Simei He <hesimei@zju.edu.cn>
@dalanlan dalanlan force-pushed the remove_job_from_tag branch from 5e69c3f to 99f6309 Compare April 19, 2015 10:40
@cpuguy83
Copy link
Member

cpuguy83 commented Apr 19, 2015

LGTM

1 similar comment
@duglin
Copy link
Contributor

duglin commented Apr 19, 2015

LGTM

duglin added a commit that referenced this pull request Apr 19, 2015
@duglin
Merge pull request #12358 from ZJU-SEL/remove_job_from_tag
b1d8ae3 
remove job from tag
@duglin duglin merged commit b1d8ae3 into moby:master Apr 19, 2015
@dalanlan dalanlan deleted the remove_job_from_tag branch April 21, 2015 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dalanlan @LK4D4 @duglin @cpuguy83 @icecrime @GordonTheTurtle