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 download #4590

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

cloudapi download #4590

wants to merge 6 commits into from

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Jan 31, 2025

This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as
    • submit a PR for the READMEs listed here
    • submit a PR for the osbuild.org website repository if this PR changed any behavior not covered by the automatically updated READMEs

@bcl bcl force-pushed the main-cloudapi-download branch from ec40673 to 64244a3 Compare January 31, 2025 23:59
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

So I wonder if we even need this. Currently we implemented downloading / copying etc… the artefact just by using cockpit-files (osbuild/image-builder-frontend#2820).

I think it might make more sense to expand the local target to accept a target artefact location / output directory instead (like ~/.cache/cockpit-image-builder/artefacts) or something.

@bcl
Copy link
Contributor Author

bcl commented Feb 3, 2025

I think we should have both. The cloudapi is incomplete without this, and may be accessed by things that can't use the local filesystem or cockpit plugins. And doing it that way feels like a hack -- it is breaking the separation between the internals and the client needing to know details like file paths.

@croissanne
Copy link
Member

croissanne commented Feb 3, 2025

I think we should have both. The cloudapi is incomplete without this, and may be accessed by things that can't use the local filesystem or cockpit plugins. And doing it that way feels like a hack -- it is breaking the separation between the internals and the client needing to know details like file paths.

Hm I think the separation is maintained as long as the path to the artefacts are resolved by composer, it's in the upload status, part of the osbuild job result. So it's not like the user is aware of where exactly the artefacts are saved, and if the artefact directory would change, the cockpit plugin wouldn't have to be updated.

I'm not sure if it's a hack to have composer output an artefact somewhere on your system. And in the case of the cockpit plugin, we have to pipe it through the cockpit api anyway, as the cockpit server might be remote. Might as well let cockpit-files handle all that. Though I agree it would be nicer to save it somewhere else, so that we don't have to point the user to /var/lib/osbuild-composer, which is an internal directory.

@bcl
Copy link
Contributor Author

bcl commented Feb 4, 2025

I'm not sure what you mean by it being in the upload status. But either way I need this in order to finish the move from weldr api to cloud api, and for composer-cli. The cloudapi is incomplete without this.

@bcl bcl force-pushed the main-cloudapi-download branch from 0b42382 to f574976 Compare February 4, 2025 17:19
@bcl bcl requested a review from croissanne February 4, 2025 17:32
@bcl
Copy link
Contributor Author

bcl commented Feb 5, 2025

I'm not sure what you mean by it being in the upload status.

LOL. I figured it out :)

@bcl bcl force-pushed the main-cloudapi-download branch from f574976 to ec82717 Compare February 5, 2025 16:36
@lucasgarfield
Copy link

@croissanne we'll also want this for composer-cli as part of our work towards deprecating the weldr api, won't we?

Copy link
Contributor

@kingsleyzissou kingsleyzissou left a comment

Choose a reason for hiding this comment

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

Thanks this look good to me, maybe @croissanne can have another look first

internal/cloudapi/v2/handler.go Show resolved Hide resolved
bcl added 6 commits February 7, 2025 12:03
This will download the build artifact from a locally saved osbuild
compose. It will set the filename to the the UUID of the build with the
artifact filename appended. eg. 1dbcc86e-745b-4061-812f-e50f06fa7cbe-disk.qcow2

Related: RHEL-60142
Even though the file isn't there it can be useful to have the full path
that it was checking for.

Related: RHEL-60142
It can include useful extra details, whether or not the artifacts
directory is setup, whether the file is missing, etc.

Related: RHEL-60142
This enables the artifact directory during the tests, it mocks up a
download artifact, and tests that it can be downloaded. The file
contains JSON because the TestRoute helper expects that as the response.

Related: RHEL-60142
@kingsleyzissou kingsleyzissou requested review from thozza and a team as code owners February 7, 2025 12:03
@kingsleyzissou kingsleyzissou requested review from mvo5 and schuellerf and removed request for a team February 7, 2025 12:03
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.

4 participants