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

Service rm #253

Merged
merged 3 commits into from
Sep 26, 2016
Merged

Service rm #253

merged 3 commits into from
Sep 26, 2016

Conversation

subfuzion
Copy link
Contributor

Closes #252

Verification

Merge #251 first.

$ make test

$ make install
$ amp service create --name pinger appcelerator/pinger
$ amp service rm pinger

$ amp service create --name pinger1 appcelerator/pinger
$ amp service create --name pinger2 appcelerator/pinger
$ amp service rm pinger1 pinger2

$ amp service create --name pinger1 appcelerator/pinger
$ amp service rm pinger1 pinger2
# verify pinger1 removed, good error for pinger2

Copy link
Contributor

@freignat91 freignat91 left a comment

Choose a reason for hiding this comment

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

Tested ok

}

message RemoveRequest {
string ident = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

why ident instead of id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

making it clear it can be an ID, short ID, or name. Actually, that was something that @freignat91 recommended. I have no problem calling it id, which matches docker docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds fine, it could use a comment to make that clear.

follow up: should RemoveResponse return an ident instead of an id?

@chrisccoy
Copy link
Contributor

Looks good, needs rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants