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.

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

Add tests for ID-based docker plugin enable/disable/rm/set #29222

Merged
merged 1 commit into from
Dec 29, 2016

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Dec 7, 2016

- What I did

This fix is a follow up based on comment:
#28789 (comment)

As #28789 has been merged in, it is possible for docker plugin inspect to search based on Name or ID Prefix. However, ID-based docker plugin enable/disable/rm/set are still not possible.

- How I did it

This fix allows docker plugin enable/disable/rm/set to search based on:

  • Full ID
  • Full Name
  • Partial ID (prefix)

- How to verify it

An additional integration test has been added to cover the changes. And all existing tests should pass.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

collection-of-christmas-kitten-wallpaper-on-wall-papers-1

This fix is a follow up of #28789.

/cc @anusha-ragunathan

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@thaJeztah
Copy link
Member

/cc @tiborvass @vieux

@tonistiigi
Copy link
Member

@yongtang This should be included in #29487 . Sorry for hijacking but it has some more critical fixes that are tied into ID requests and couldn't wait. Can you double check that that PR fixes this use case and if everything is ok, rebase your PR to only add the test.

@yongtang yongtang force-pushed the 28789-plugin-inspect-follow-up branch from 336af27 to 4d9ffca Compare December 22, 2016 23:43
@yongtang
Copy link
Member Author

@tonistiigi Thanks. I checked the PR #29487 and verified that it addressed the issue. The following is the test pass on top of #29487:

INFO: Testing against a local daemon
PASS: docker_cli_plugins_test.go:269: DockerSuite.TestPluginIDPrefix	4.554s
OK: 1 passed
PASS

I also rebased this PR so that now it only includes tests. The test for this PR will fail temporarily but should pass once PR #29487 is merged.

@yongtang yongtang force-pushed the 28789-plugin-inspect-follow-up branch from 4d9ffca to 0f1756f Compare December 24, 2016 13:53
@yongtang yongtang force-pushed the 28789-plugin-inspect-follow-up branch from 0f1756f to 3420c6e Compare December 27, 2016 04:48
@tonistiigi
Copy link
Member

@yongtang Needs rebase

This fix is a follow up based on comment:

and a follow up to:
moby#29222 (comment)

As moby#28789 has been merged in, it is possible for `docker plugin inspect`
to search based on Name or ID Prefix. However, ID-based
`docker plugin enable/disable/rm/set` are still not possible.

This fix addes test for  `docker plugin enable/disable/rm/set` to search based on:
- Full ID
- Full Name
- Partial ID (prefix)

The actual fix is done in  moby#29487.

This fix is a follow up of moby#28789 and  moby#29487.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 28789-plugin-inspect-follow-up branch from 3420c6e to c80e74e Compare December 28, 2016 22:17
@yongtang
Copy link
Member Author

Thanks @tonistiigi The PR has been rebased.

@tonistiigi
Copy link
Member

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit 00cdb97 into moby:master Dec 29, 2016
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Dec 29, 2016
@yongtang yongtang deleted the 28789-plugin-inspect-follow-up branch December 29, 2016 14:14
@thaJeztah thaJeztah changed the title Allow ID-based docker plugin enable/disable/rm/set Add tests for ID-based docker plugin enable/disable/rm/set Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants