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

cli/command/container: implement docker run --annotation #4156

Merged
merged 1 commit into from
May 5, 2023

Conversation

AkihiroSuda
Copy link
Collaborator

- What I did
Implemented docker run --annotation.

For moby/moby#45025 (Docker v24, API v1.43).

docker run --annotation foo=bar is similar to podman run --annotation foo=bar, however, unlike Podman, Docker implementation also accepts an annotation with an empty value. (docker run --annotation foo)

- How I did it

Added the --annotation flag as an *opts.MapOpts.

- How to verify it

Run docker run --annotation foo=bar, and run docker inspect to confirm that the annotation is set.

- Description for the changelog

cli/command/container: implement docker run --annotation

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

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2023

Codecov Report

Merging #4156 (88be16c) into master (60d0659) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4156   +/-   ##
=======================================
  Coverage   58.86%   58.87%           
=======================================
  Files         572      572           
  Lines       49544    49556   +12     
=======================================
+ Hits        29162    29174   +12     
  Misses      18616    18616           
  Partials     1766     1766           

@thaJeztah
Copy link
Member

Thanks! This should probably be fine, but before we merge, I want to do a quick check look; ISTR we discussed some of this, and whether or not to add this "as-is", have a "porcelain" option (for runtime annotations), etc. (But it's been some time, so I need to read up / read back on some of that).

@AkihiroSuda
Copy link
Collaborator Author

Thanks! This should probably be fine, but before we merge, I want to do a quick check look; ISTR we discussed some of this, and whether or not to add this "as-is", have a "porcelain" option (for runtime annotations), etc. (But it's been some time, so I need to read up / read back on some of that).

This feature has been already implemented in podman run --annotation, so my opinion is to just follow their CLI design so that their users can easily adopt docker run --annotation too

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

Completions LGTM, thanks.

@AkihiroSuda
Copy link
Collaborator Author

@thaJeztah Can we merge this?

@thaJeztah
Copy link
Member

We discussed this in the maintainers meeting, and generally are ok with the current implementation. There were some concerns;

  • Is "annotation" confusing (vs "label")? We could probably borrow some of the wording from https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/ to explain the differences
  • Is it clear enough this is passed through to the runtime? We discussed describing it as "stored on the container, and passed through to the OCI runtime"
  • We discussed the = being optional; we don't expect these to (like --env) ever take values from the environment, and that to be ambiguous, so we're good with that (easier to use)
  • We do want @oyabun to have a quick look before merging.

Thanks!

For moby/moby PR 45025 (Docker v24, API v1.43).

`docker run --annotation foo=bar` is similar to `podman run --annotation foo=bar`,
however, unlike Podman, Docker implementation also accepts an annotation with an empty value.
(`docker run --annotation foo`)

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
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

I haven't seen strong voices about alternative naming for the flag, so I'll bring this one in. Thanks!

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Apr 8, 2024
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.

7 participants