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

builder: use cloud api #73

Merged
merged 4 commits into from
Feb 11, 2022
Merged

builder: use cloud api #73

merged 4 commits into from
Feb 11, 2022

Conversation

gicmo
Copy link
Contributor

@gicmo gicmo commented Jan 26, 2022 •
edited
Loading

Port the plugin to use the Composer's cloud API.

Needs osbuild/osbuild-composer#2214

@gicmo gicmo marked this pull request as draft January 26, 2022 20:20
@gicmo gicmo force-pushed the cloudapi branch 5 times, most recently from cb580ce to f83ef48 Compare February 2, 2022 12:42
@gicmo gicmo marked this pull request as ready for review February 2, 2022 15:28
@gicmo
Copy link
Contributor Author

gicmo commented Feb 2, 2022

Composer PR landed, this is ready for review.

@teg
Copy link
Member

teg commented Feb 2, 2022

Reading through this, I realised we may not have enabled all the necessary image types in the cloud api in composer? And the names of some will have changed, where should we do the backwards compatible translation? In the koji plugin?

@gicmo
Copy link
Contributor Author

gicmo commented Feb 2, 2022

Reading through this, I realised we may not have enabled all the necessary image types in the cloud api in composer?

By looking at openapi.v2.yml I think we at least miss ec2-*.

And the names of some will have changed, where should we do the backwards compatible translation? In the koji plugin?

I think it makes sense to have it here, maybe switch the over in Pungi and then remove the translation layer later here.

@teg
Copy link
Member

teg commented Feb 3, 2022

That would be perfect I think

croissanne
croissanne previously approved these changes Feb 7, 2022
View reviewed changes
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.

lgtm

plugins/builder/osbuild.py Show resolved Hide resolved
achilleas-k
achilleas-k previously approved these changes Feb 7, 2022
View reviewed changes
gicmo added 2 commits February 10, 2022 12:20
@gicmo
builder: use cloud api
cc7843d 
Composer now[1] has integrated the koji API into the "cloud API"
and thus we can use this more general purpose and powerful API
instead of using the specialized koji API endpoint.
Adapt the request and response structures as well as the unit
tests to use that.

[1] PR #2214, commit 11e2ae45284bfb0d89ef1c1e0d2aa4ae230ea573
@gicmo
test/builder: validate image type in request
a82ac3b 
Take the current list of valid image types currently supported by
the cloud api and validdate it during the compose request. Also
allow a test "image_type" image type which is used all over the
place in the testing code.
@gicmo gicmo dismissed stale reviews from achilleas-k and croissanne via 8e3be23 February 10, 2022 12:23
@gicmo gicmo requested a review from thozza February 10, 2022 12:23
gicmo added 2 commits February 10, 2022 12:26
@gicmo
builder: map koji api image types to cloud ones
e24f2f4 
Map the image types used by the koji API to the image types used
by the cloud api. This should allow for a smooth transition when
the plugin is upgraded, i.e. the pungi configuration can be used
unmodified. After all the plugins are upgraded the pungi config
should be changed to use the native image types and then this
mapping could be removed again.
@gicmo
schutzbot: pin composer (need integrated API)
2989242 
Update composer to a commit that includes the new integrated
cloud API as well as exposing all the image types via it.
teg
teg approved these changes Feb 10, 2022
View reviewed changes
Copy link
Member

@teg teg left a comment

Choose a reason for hiding this comment

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

Love it!

thozza
thozza approved these changes Feb 11, 2022
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.

Nice work! I have one non-blocking question...

test/unit/test_builder.py Show resolved Hide resolved
@gicmo
Copy link
Contributor Author

gicmo commented Feb 11, 2022

I just double checked that we get a new OSBuildImage for every new task:

In method

def takeTask(self, task):

we get the handler class (OSBuildImage(BaseTaskHandler)) in our case, via:

handlerClass = self.handlers[method]

and then a new instance is invoked:

handler = handlerClass(task_info['id'], method, params, self.session, self.options)

matching our case def __init__(self, task_id, method, params, session, options):

@gicmo gicmo merged commit ce21817 into osbuild:main Feb 11, 2022
@gicmo gicmo deleted the cloudapi branch February 11, 2022 15:36
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

@teg teg teg approved these changes

@croissanne croissanne croissanne approved these changes

@achilleas-k achilleas-k Awaiting requested review from achilleas-k

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

Successfully merging this pull request may close these issues.
5 participants
@gicmo @teg @achilleas-k @thozza @croissanne