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 one-shot to container API stats #3089

Merged
merged 5 commits into from
Feb 16, 2023

Conversation

aroxby-wayscript
Copy link
Contributor

Add the one-shot parameter to container stats to enable pulling just a single stats update. New in docker engine 20.10.0.

See the moby PR: moby/moby#40478

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Two adjustments:

  1. Since this was added in v1.41, we should check the API version if the param is set, which will realistically mean changing the default to one_shot=None. See an example in another API call
  2. one_shot=True is only valid if stream=False, so we should check that as an invariant here and raise an exception, as otherwise it'll be rather hard to debug because of the stream helper in some cases (normally I'd say rely on server-side validation, but in this case there's no sensible meaning to the combo, so it seems safe enough to add a redundant check for)

@aroxby-wayscript
Copy link
Contributor Author

@milas, thanks for that very helpful and clear feedback. I believe this is what you'd want to see.

@aroxby-wayscript
Copy link
Contributor Author

@milas just pinging you again on this. It looks like everything is setup now. What do you think?

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

One small tweak but LGTM otherwise. Thanks for making the changes!

docker/api/container.py Outdated Show resolved Hide resolved
aroxby-wayscript and others added 4 commits February 3, 2023 15:06
Signed-off-by: Andy Roxby <107427605+aroxby-wayscript@users.noreply.github.com>
Signed-off-by: Andy Roxby <107427605+aroxby-wayscript@users.noreply.github.com>
Signed-off-by: Andy Roxby <107427605+aroxby-wayscript@users.noreply.github.com>
Co-authored-by: Milas Bowman <devnull@milas.dev>
Signed-off-by: Andy Roxby <107427605+aroxby-wayscript@users.noreply.github.com>
Signed-off-by: Andy Roxby <107427605+aroxby-wayscript@users.noreply.github.com>
@aroxby-wayscript
Copy link
Contributor Author

@milas thanks again for all your helpful feedback. How are we looking on this?

@aroxby-wayscript
Copy link
Contributor Author

@milas just sending another ping on this. Look good?

@milas milas merged commit e9d4ddf into docker:main Feb 16, 2023
@aroxby-wayscript aroxby-wayscript deleted the container-stats-one-shot branch February 16, 2023 15:51
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.

2 participants