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 Logs to ContainerAttachOptions #26718

Merged
merged 1 commit into from Oct 27, 2016

Conversation

@ncdc
Copy link
Contributor

commented Sep 19, 2016

- What I did
Added Logs to ContainerAttachOptions so go clients can request to retrieve container logs as part of the attach process.

- How I did it
N/A

- How to verify it
Create a go client and attempt to attach to a container with Logs set to true. You should get container logs whereas before you wouldn't. I'd be happy to write a test for this if you can point me at the appropriate file to do this in.

- Description for the changelog

Add Logs to ContainerAttachOptions

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

Signed-off-by: Andy Goldstein agoldste@redhat.com

cc @runcom

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2016

SGTM
A test would be great. Probably needs to go in integration-cli.

@runcom

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

👍 having a test would be great yeah

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2016

Do you want me to add --logs as an option to docker attach as well?

@ncdc ncdc force-pushed the ncdc:add-logs-to-container-attach-options branch from 0c76386 to ded87dc Sep 20, 2016
@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

I added an integration test. It isn't super clean but it should do the job. PTAL!

@ncdc ncdc force-pushed the ncdc:add-logs-to-container-attach-options branch from ded87dc to 695a790 Sep 20, 2016
@LK4D4

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

@ncdc I think this should be power-user stuff. So I think it's not necessary to add --logs to attach.

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

@LK4D4 I agree, and if we need a CLI flag, it can be in a follow-up.

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

Looks like my new test is failing in win2lin due to an https/http mismatch trying to talk to the daemon. Any suggestions how to make this work everywhere?

@runcom

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

Ping @jhowardmsft can you help on win2lin?

@lowenna

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

Wow. No idea what that is. Could just be a blip. Restarting.

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

I added a new integration test that creates a client via client.NewEnvClient() so maybe there's something particular about how the daemon is set up in the windows vs non-windows environments?

@lowenna

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

@tiborvass Could this be something you changed on those servers?

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

@jhowardmsft @tiborvass can you help me figure out why this is failing win2lin?

cc @jwhonce @runcom @mrunalp we need this to fix some issues in Kubernetes/OpenShift

@runcom runcom added this to the 1.13.0 milestone Oct 19, 2016
@runcom

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

@ncdc can you rebase and push force here? that failure in win2lin seems unrelaed so maybe it got fixed in master sorry I missed the actual test failure.

Andy Goldstein
Signed-off-by: Andy Goldstein <agoldste@redhat.com>
@ncdc ncdc force-pushed the ncdc:add-logs-to-container-attach-options branch from 695a790 to fc8097f Oct 19, 2016
@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

@runcom rebased

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

Yay all tests passed!

@LK4D4

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

LGTM

@runcom

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

Woa so the rebase actually fixed the error.

LGTM

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

It looks like the test that was previously failing on windows is now being skipped maybe?

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

This needs docs updates as well;

@mlaventure

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Shouldn't we have the same options we do have now with docker logs?

If I'm attaching to a container with several MB of logs, I may just want to see the last n lines and more important not have to wait until all those MB get transferred before being able to interact with my container.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

let me temporarily move this back to design-review for discussing @mlaventure's suggestion

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

I'm happy to do whatever makes the most sense.

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

Although I would recommend doing that in a separate PR, as the http handler/router in the daemon already supports the "logs" param, so this is just enabling it in the api client. It does not currently support what @mlaventure suggested.

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

@thaJeztah re docs updates, this isn't actually changing anything in the remote API. It's just adding a parameter to the client package that is already supported by the remote API. Are there docs that need updating for client package changes?

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Oct 22, 2016

@ncdc if this property is already documented in the API JSON examples, then it's not needed, but could you check if that's the case?

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

@thaJeztah @mlaventure @runcom where do we stand on this?

@runcom

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

LGTM, not sure whether docs are OK though

@ncdc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

docs should be ok as best I can tell

On Thu, Oct 27, 2016 at 2:18 PM, Antonio Murdaca notifications@github.com
wrote:

LGTM, not sure whether docs are OK though


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26718 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAABYqFcILcjCz2SmFE6YZS9rhllO86zks5q4OsSgaJpZM4KA2aw
.

@mlaventure

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

LGTM

@mlaventure mlaventure merged commit 4d2e258 into moby:master Oct 27, 2016
4 checks passed
4 checks passed
docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25124 has succeeded
Details
janky Jenkins build Docker-PRs 33721 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 4575 has succeeded
Details
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
…options

Add Logs to ContainerAttachOptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.