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

build: make annotations work with push flag #2098

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Oct 26, 2023

related to #2020

When implementing annotations in our build-push-action (docker/build-push-action#992), I found out they were not working: https://github.com/docker/build-push-action/actions/runs/6642453793/job/18047273130#step:7:24

Annotations work with --output but it doesn't when using --push. This moves the parsing annotations logic to the controller so we can set annotations after --push has been converted to an export entry.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max added this to the v0.12.0 milestone Oct 26, 2023
Copy link
Collaborator

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

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

Seems fine. I'd just like to see the line where --push get translated so I can better understand how things fit together.

Comment on lines -179 to -184
for _, e := range opts.Exports {
for k, v := range annotations {
e.Attrs[k.String()] = v
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

So here --push hasn't been translated to an export entry. Can you just link me the line where this happens for learning purposes?

Comment on lines +140 to +144
for _, o := range outputs {
for k, v := range annotations {
o.Attrs[k.String()] = v
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's delayed here (so --push is a true alias for --output=type=registry) and that's why this works.

@favonia
Copy link

favonia commented Oct 26, 2023

Please ignore my previous (deleted) comment, which is nonsensical.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

It looks like a better approach would be to remove ExportPush/ExportLoad from controller (and then likely new Annotations as well). But controller API is not backwards compatible so not a blocker.

@tonistiigi tonistiigi merged commit 2f1b7a0 into docker:master Oct 26, 2023
59 checks passed
@crazy-max crazy-max deleted the annotations-push branch October 26, 2023 20:09
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.

None yet

5 participants