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

v1: remove upload request #420

Closed
wants to merge 1 commit into from
Closed

v1: remove upload request #420

wants to merge 1 commit into from

Conversation

chloenayon
Copy link
Contributor

@chloenayon chloenayon commented Sep 7, 2022 •
edited
Loading

Fixes #368

@app-sre-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@chloenayon chloenayon force-pushed the inferuploadrequest branch 2 times, most recently from af4ae4b to d72e64c Compare September 12, 2022 12:57
croissanne
croissanne reviewed Sep 19, 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.

Why mark upload_request as not required? I think this PR is missing some context?

internal/v1/api.yaml
@@ -395,7 +395,6 @@ components:
required:
- architecture
- image_type
- upload_request
Copy link
Member

Choose a reason for hiding this comment

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

Upload request is required, no?

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 should've added it to the description, this is intended to address #368 , there are cases in which upload request is not necessary and can be inferred from the image type

internal/v1/server.go
@@ -507,7 +507,7 @@ func (h *Handlers) ComposeImage(ctx echo.Context) error {
return err
}

if (composeRequest.ImageRequests[0].UploadRequest == UploadRequest{}) {
if (composeRequest.ImageRequests[0].UploadRequest == &UploadRequest{}) {
Copy link
Member

@croissanne croissanne Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This check will fail if there's an empty upload request object, but not on nil.

@ondrejbudai
Copy link
Member

Sorry for ignoring this PR for so long, I wasn't sure what to do about it until today.

While I'm very much for any simplification, this has the disadvantage that it's no longer obvious from the spec that upload_options are still required for EC2, GCP and Azure. I think that I prefer the spec being obvious rather than the API calls being simple.

I actually proposed a change to the cloud API in composer today that makes upload_options required for just some image types - osbuild/osbuild-composer#3018. If the reviewers like this change, I suppose we can make the same changes here and make all upload options required except for the S3 one, which indeed isn't needed.

@croissanne croissanne closed this Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@croissanne croissanne croissanne left review comments

@ondrejbudai ondrejbudai Awaiting requested review from ondrejbudai

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

Successfully merging this pull request may close these issues.

API v1 can be simplified for s3 images
4 participants
@chloenayon @app-sre-bot @ondrejbudai @croissanne