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

cloudapi: support worker server target result options #4574

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

croissanne
Copy link
Member

Adds results for the worker server target, the full path to the artefact. This is useful for cockpit-image-builder, which can then show the user the path to the image they have built in case they selected a local target.

@croissanne croissanne requested review from ondrejbudai, bcl and thozza and removed request for ondrejbudai, bcl and thozza January 23, 2025 10:06
@croissanne croissanne force-pushed the workerservertargetfix branch from a008a41 to 4c0d71a Compare January 23, 2025 10:26
bcl
bcl previously approved these changes Jan 23, 2025
Copy link
Contributor

@bcl bcl left a comment

Choose a reason for hiding this comment

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

Looks good to me. There is one small issue, that may not really matter, but to save it locally you now need an entry in upload_targets where before you could just set upload_options on the image_request. If you do that now it defaults to an S3 upload and then fails because it isn't setup -- but doesn't fail until the end. So we may want to add a check for this at some point.

      "upload_targets": [
        {
          "type": "local",
          "upload_options": {
            "local_save": true
          }
        }
      ],

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.

Thanks. The PR looks good in general. 👍

However, I do have a few nitpicks... 😇 Specifically, I'd like to request changes to the naming of the newly added struct member, so that it expresses what value it actually holds. IOW don't use "filename" if it holds relative or absolute paths to the artifact. We use "filename" when it is really just the name of the file.

internal/cloudapi/v2/imagerequest.go Outdated Show resolved Hide resolved
cmd/osbuild-worker/jobimpl-osbuild.go Outdated Show resolved Hide resolved
internal/cloudapi/v2/openapi.v2.yml Outdated Show resolved Hide resolved
In order to get the artifact location from the cloudapi, add a helper
function in the worker server.
The local target shouldn't require any specific configuration and should
just be available always.
@croissanne croissanne force-pushed the workerservertargetfix branch from d4f0010 to 5592882 Compare January 24, 2025 09:58
@croissanne croissanne requested review from thozza and bcl January 24, 2025 09:58
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.

Thank you!

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Thanks!

@croissanne croissanne merged commit a44a499 into osbuild:main Jan 24, 2025
45 of 46 checks passed
@croissanne croissanne deleted the workerservertargetfix branch January 24, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants