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: Add architecture support to the optional Blueprint #4374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Sep 19, 2024

If included it overrides the architecture in the compose image request.

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-bp-arch branch 2 times, most recently from d98f127 to f33ebc3 Compare October 15, 2024 21:12
Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 15, 2024
@bcl bcl removed the Stale label Nov 15, 2024
@bcl bcl force-pushed the main-bp-arch branch 2 times, most recently from 0018048 to 018648f Compare November 15, 2024 18:53
@bcl bcl requested a review from ondrejbudai November 26, 2024 23:28
@bcl bcl requested a review from thozza December 17, 2024 18:13
@bcl
Copy link
Contributor Author

bcl commented Dec 17, 2024

Changed this to address @thozza 's comments about arch handling in #4390

achilleas-k
achilleas-k previously approved these changes Jan 13, 2025
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.

While this PR is OK from the implementation point of view, I don't like it from the architecture point of view...

I'm still not fully on board with the plan to add the architecture to the CloudAPI version of BP. The reason is that it duplicates the information already specified in the ImageRequest

type ImageRequest struct {
Architecture string `json:"architecture"`
ImageType ImageTypes `json:"image_type"`
Ostree *OSTree `json:"ostree,omitempty"`
Repositories []Repository `json:"repositories"`
// Size of image, in bytes. When set to 0 the image size is a minimum
// defined by the image type.
Size *uint64 `json:"size,omitempty"`
// Options for a given upload destination.
// This should really be oneOf but AWSS3UploadOptions is a subset of
// AWSEC2UploadOptions. This means that all AWSEC2UploadOptions objects
// are also valid AWSS3UploadOptionas objects which violates the oneOf
// rules. Therefore, we have to use anyOf here but be aware that it isn't
// possible to mix and match more schemas together.
UploadOptions *UploadOptions `json:"upload_options,omitempty"`
// The type and options for multiple upload targets. Each item defines
// a separate upload destination with its own options. Multiple
// different targets as well as multiple targets of the same kind are
// supported.
UploadTargets *[]UploadTarget `json:"upload_targets,omitempty"`
}

Personally, I think that the fact that arch was added to the Weldr API BP version was a mistake. Instead, it should have been added to the ComposeRequest

type ComposeRequest struct {
BlueprintName string `json:"blueprint_name"`
ComposeType string `json:"compose_type"`
Size uint64 `json:"size"`
OSTree *ostree.ImageOptions `json:"ostree,omitempty"`
Branch string `json:"branch"`
Upload *uploadRequest `json:"upload"`
}
struct for the Compose API call option when building an image for a specific BP.

Hardcoding the arch in the BP itself IMHO makes it less flexible to use for building various image types, and potentially for various arches. Architecture should be IMO treated in the same way as image type - you specify the BP name and when building the image, you pick the image type and an architecture.

I'm worried that we will make things more confusing and architecturally wrong if we add architecture to the BP in Cloud API.

I'd like to have a broader dicsussion and a consensus before we merge this.

@bcl
Copy link
Contributor Author

bcl commented Jan 13, 2025

All I'm trying to do here is make things consistent. Arch ALREADY exists in the blueprint, except that it wasn't added to cloudapi, so the current situation is that it's more confusing.
I don't think this is the place to discuss whether or not it should have been added, it is already there.
Also note that the arch is optional, so it only makes things less flexible if it has been set and that's under the control of the user. Maybe they have a blueprint they only want to use on a specific arch.

The blueprint may have the architecture set. If set it must match the
architecture set by the request. This ensures that clients have
correctly set the architecture and prevents unexpectedly ignoring the
blueprint architecture.

With the WELR API the blueprint architecture would override the host
arch. But with the Cloud API there is no host arch to override so the
request must have the expected architecture set as part of the request.

Related: RHEL-60125
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.

3 participants