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.

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

Use distro repos in cloudapi #3928

Merged
merged 3 commits into from
Mar 11, 2024
Merged

Use distro repos in cloudapi #3928

merged 3 commits into from
Mar 11, 2024

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Feb 3, 2024

This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as

thozza
thozza reviewed Feb 5, 2024
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.

Overall, this looks good. 👍 I added one question, but nothing serious.

internal/cloudapi/v2/handler.go Outdated Show resolved Hide resolved
internal/cloudapi/v2/v2_test.go Outdated Show resolved Hide resolved
@bcl bcl force-pushed the main-cloudapi-repos branch 3 times, most recently from cb5778e to 709c8ca Compare February 12, 2024 16:06
@bcl bcl requested a review from thozza February 12, 2024 16:07
@mcattamoredhat mcattamoredhat mentioned this pull request Feb 13, 2024
Open
@bcl bcl force-pushed the main-cloudapi-repos branch 4 times, most recently from 5cd4025 to cae1af9 Compare February 20, 2024 23:44
@bcl bcl requested a review from croissanne February 27, 2024 19:06
@bcl bcl force-pushed the main-cloudapi-repos branch 2 times, most recently from 240f5a4 to 641ec3c Compare March 1, 2024 16:29
thozza
thozza reviewed Mar 4, 2024
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.

I found one problem that I commented on.

Other than that, I'm wondering if it would be worth to extend some of the existing functional test cases to also cover this feature, or at least adding a new unit test for it.

The rest looks good 👍

internal/cloudapi/v2/compose.go Outdated Show resolved Hide resolved
@bcl bcl force-pushed the main-cloudapi-repos branch 2 times, most recently from 2568bdd to ef8fe72 Compare March 5, 2024 17:58
@bcl
Copy link
Contributor Author

bcl commented Mar 5, 2024

I'm wondering if it would be worth to extend some of the existing functional test cases to also cover this feature, or at least adding a new unit test for it.

Added a unit test to check it. I had to hard-code a path traversal so I could load the repos, it seems stable, but let me know if there's a better way.

@supakeen supakeen self-requested a review March 6, 2024 21:55
supakeen
supakeen approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Neat.

thozza
thozza approved these changes Mar 8, 2024
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.

Thanks for adding the test case.

internal/cloudapi/v2/compose_test.go Show resolved Hide resolved
bcl added 3 commits March 8, 2024 15:35
@bcl
Move RepoRegistry setup into Composer
e450bd6 
This is so that both the weldr and cloud api's can use it as the source
of their repositories.
@bcl
cloudapi: Pass the RepoRegistry to the cloudapi Server
f4287a9
@bcl
cloudapi: Use distro repos if none included in imageRequest
73487f2 
In order to support cloudapi blueprint requests from the cmdline using
composer-cli it needs to select the repositories based on the selected
distribution instead of requiring the user to include them with the
request.

If the image request includes repositories they are used, which matches
the current behavior. If the repository list is empty it will use the
distribution name to select from the repositories shipped with
osbuild-composer.
@bcl bcl enabled auto-merge (rebase) March 8, 2024 23:36
@bcl bcl merged commit 57ebfb4 into osbuild:main Mar 11, 2024
59 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thozza thozza thozza approved these changes

@supakeen supakeen supakeen approved these changes

@croissanne croissanne Awaiting requested review from croissanne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bcl @thozza @supakeen