Skip to content

Allow setting custom tags when uploading to AWS #327

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 24, 2025
Merged

Allow setting custom tags when uploading to AWS #327

merged 1 commit into from
Oct 24, 2025
+17 −0

Conversation

@FrostyX
Copy link
Contributor

@FrostyX FrostyX commented Sep 25, 2025 •
edited
Loading

@FrostyX FrostyX requested a review from a team as a code owner September 25, 2025 14:02
@FrostyX FrostyX requested review from croissanne, mvo5 and thozza and removed request for a team September 25, 2025 14:02
@FrostyX
Copy link
Contributor Author

FrostyX commented Sep 25, 2025

I based the PR on top of #300 to make my life easier. Once that PR gets merged, I will rebase so this one has just one commit.

@FrostyX FrostyX mentioned this pull request Sep 25, 2025
Merged
@FrostyX FrostyX force-pushed the aws-tag branch 3 times, most recently from 6abdc52 to 478f507 Compare October 1, 2025 07:32
@FrostyX FrostyX mentioned this pull request Oct 1, 2025
Merged
@thozza
Copy link
Member

thozza commented Oct 1, 2025

@FrostyX You'll need to update the vendored osbuild/images to https://github.com/osbuild/images/releases/tag/v0.200.0

@FrostyX
Copy link
Contributor Author

FrostyX commented Oct 1, 2025

Thank you @thozza,
I was puzzled what's wrong with it.

Updated.

@thozza
Copy link
Member

thozza commented Oct 2, 2025

@FrostyX, you need to bump the minimum required version of osbuild in the SPEC file, because the vendored osbuild/images library requires it (See the SPEC file check).

@FrostyX
Copy link
Contributor Author

FrostyX commented Oct 2, 2025 •
edited
Loading

Some of the checks run for quite a long time, so I don't know yet. But hopefully, third time's the charm.

Thank you @thozza

@FrostyX FrostyX mentioned this pull request Oct 2, 2025
Closed
thozza
thozza previously approved these changes Oct 2, 2025
View reviewed changes
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM

mvo5
mvo5 reviewed Oct 6, 2025
View reviewed changes
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This is fine but see osbuild/images#1915 (comment) - I'm not a fan of the [2]string to represent key/val. Not a blocker, I don't want to slow you down but I would like to tweak this at some point.

Also a tiny question inline.

cmd/image-builder/upload.go Outdated
var slicedTags [][2]string
for _, tag := range tags {
parts := strings.SplitN(tag, "=", 2)
if len(parts) != 2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the policy of AWS here? Are values wit = allowed? Because if they are we could change this to if len(parts) < 2 and allow = in the values as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am still testing this part of the PR but then I will rebase :-)

Copy link
Contributor Author

@FrostyX FrostyX Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It allows = in values, so I changed the code like you suggested.

@supakeen
Copy link
Member

supakeen commented Oct 7, 2025 •
edited
Loading

This is fine but see osbuild/images#1915 (comment) - I'm not a fan of the [2]string to represent key/val. Not a blocker, I don't want to slow you down but I would like to tweak this at some point.

Also a tiny question inline.

We can do a potential struct follow-up ourselves so we don't have @FrostyX juggle too many dependent PRs.

This PR needs some updates to the go deps (probably because we bumped versions in main) but otherwise looks fine.

@FrostyX
Copy link
Contributor Author

FrostyX commented Oct 8, 2025

I submitted a PR for the AWSTag struct
osbuild/images#1939

@mvo5
Copy link
Collaborator

mvo5 commented Oct 8, 2025

I submitted a PR for the AWSTag struct osbuild/images#1939

Super nice, thank you! Probably worth cleaning the commit history (there is a wip commit in there) when doing the update once #1939 is merged.

@FrostyX
Copy link
Contributor Author

FrostyX commented Oct 8, 2025

I rebased the PR. I think it will require some go.mod update once the images PR gets merged but otherwise I think it is ready.

@supakeen supakeen requested a review from lzap October 24, 2025 06:57
@supakeen supakeen added this pull request to the merge queue Oct 24, 2025
Merged via the queue into osbuild:main with commit ee1d076 Oct 24, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mvo5 mvo5 mvo5 left review comments

@lzap lzap lzap approved these changes

@supakeen supakeen supakeen approved these changes

@thozza thozza thozza left review comments

@croissanne croissanne Awaiting requested review from croissanne croissanne is a code owner automatically assigned from osbuild/osbuild-reviewers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow setting tags when uploading to AWS

5 participants

@FrostyX @thozza @supakeen @mvo5 @lzap