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 support for annotations #22124

Merged
merged 1 commit into from Mar 25, 2024
Merged

Conversation

diplane
Copy link
Contributor

@diplane diplane commented Mar 21, 2024

In addition to the issue in docker-compose and the corresponding CL:
docker/compose#11644
docker/compose#11645

Compose doesn't send Annotations as part of the HostConfig to the docker/podman socket.

On the other hand, podman also ignores Annotations received from the create container API handle.

This CL fix that.

None

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Mar 21, 2024
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Mar 21, 2024
@openshift-ci openshift-ci bot added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Mar 21, 2024
@mheon
Copy link
Member

mheon commented Mar 21, 2024

You need to add a test, but the code change looks good.

@diplane
Copy link
Contributor Author

diplane commented Mar 21, 2024

You need to add a test, but the code change looks good.

I wrote a test, but also found that podman inspect <container> doesn't output annotations, but docker inspect <container> does. I think it's not a bad idea to have similar behavior, so I've fixed that as well.
Just one thing which I am not sure: is it necessary to output annotations that podman adds itself.

@diplane
Copy link
Contributor Author

diplane commented Mar 22, 2024

/retest

Copy link
Contributor

openshift-ci bot commented Mar 22, 2024

@diplane: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mheon
Copy link
Member

mheon commented Mar 22, 2024

Quay broken.
Error: creating build container: initializing source docker://ubuntu:latest: Requesting bearer token: invalid status code from registry 502 (Bad Gateway)
Manually restarted all

@diplane
Copy link
Contributor Author

diplane commented Mar 22, 2024

Quay broken. Error: creating build container: initializing source docker://ubuntu:latest: Requesting bearer token: invalid status code from registry 502 (Bad Gateway) Manually restarted all

One test still failed with same reason
Error: initializing source docker://quay.io/libpod/testimage:20240123: reading manifest 20240123 in quay.io/libpod/testimage: received unexpected HTTP status: 504 Gateway
Can you restart it again?

@mheon
Copy link
Member

mheon commented Mar 22, 2024

Done

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Can you please squash your commits and add a proper commit message because something like Add tests is not useful for anybody reading the git log.
Your PR description is fine but that is something that should be part of the commit message.

libpod/container_inspect.go Outdated Show resolved Hide resolved
Fix following issues:
- create container API handler ignores Annotations from HostConfig
- inspect container API handler does not provide Annotations as
  part of HostConfig

Signed-off-by: diplane <diplane3d@gmail.com>
@mheon
Copy link
Member

mheon commented Mar 22, 2024

LGTM

@mheon
Copy link
Member

mheon commented Mar 22, 2024

@containers/podman-maintainers PTAL

@diplane diplane requested a review from Luap99 March 22, 2024 22:05
diplane

This comment was marked as off-topic.

diplane

This comment was marked as off-topic.

@mheon
Copy link
Member

mheon commented Mar 25, 2024

@baude @rhatdan PTAL

@baude
Copy link
Member

baude commented Mar 25, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2024
@baude
Copy link
Member

baude commented Mar 25, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Mar 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, diplane

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e64d814 into containers:main Mar 25, 2024
93 checks passed
@diplane diplane deleted the annotations branch March 27, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants