Add exec inspect by tasuki · Pull Request #127 · docker-php/docker-php · GitHub
Skip to content
This repository was archived by the owner on Oct 26, 2019. It is now read-only.

Add exec inspect#127

Merged
ubermuda merged 4 commits into
docker-php:masterfrom
tasuki:add-container-exec-inspect
Sep 3, 2015
Merged

Add exec inspect#127
ubermuda merged 4 commits into
docker-php:masterfrom
tasuki:add-container-exec-inspect

Conversation

@tasuki

@tasuki tasuki commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

No description provided.

@ubermuda

ubermuda commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

@tasuki

tasuki commented Sep 2, 2015

Copy link
Copy Markdown
Contributor Author

@ubermuda: Done! The build fails for some unrelated reason.

@ubermuda

ubermuda commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

Thank you! Could you move it to a test by itself though? Something like testExecInspect. This way we have a better way of what's going on in case of a problem.

@tasuki

tasuki commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

Separated the test!
(I just thought there'd be some unnecessary duplication, also the tests are hella slow.)

@ubermuda

ubermuda commented Sep 3, 2015

Copy link
Copy Markdown
Contributor

No problem that's a legitimate though, I just favour separation of tests over speed right now. They are indeed slow because they are not "unit tests" per se (they talk to a real docker daemon), and we sometimes need to blatantly sleep() to wait for a response, so yeah...

Anyway, thanks again for your contribution!

ubermuda added a commit that referenced this pull request Sep 3, 2015
@ubermuda
Merge pull request #127 from tasuk/add-container-exec-inspect
2926fcd 
Add exec inspect
@ubermuda ubermuda merged commit 2926fcd into docker-php:master Sep 3, 2015
@tasuki tasuki deleted the add-container-exec-inspect branch September 3, 2015 14:25
@tasuki

tasuki commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

Thanks for the quick merge! If you could do a new release, that'd be absolutely smashing :)

@ubermuda

ubermuda commented Sep 3, 2015

Copy link
Copy Markdown
Contributor

Sure thing! Just pushed v0.4.3

@tasuki

tasuki commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

🍻

sroze
sroze reviewed Sep 3, 2015
View reviewed changes
Comment thread src/Docker/Manager/ContainerManager.php

@sroze sroze Sep 3, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's inconsistent with the other methods that returns a ResponseInterface object btw 👀

@ubermuda ubermuda Sep 3, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the case of exec() you might need/want to interact with the output as a stream or whatever, which is not the case here, or did I miss something?

@tasuki tasuki Sep 7, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just some methods return ResponseInterface. There's plenty of strings/arrays being returned too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

3 participants

@tasuki @ubermuda @sroze