-
Notifications
You must be signed in to change notification settings - Fork 8
Add optional architecture field #3198
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
base: main
Are you sure you want to change the base?
Conversation
operator/config/crd/bases/embeddedcluster.replicated.com_installations.yaml
Outdated
Show resolved
Hide resolved
operator/config/crd/bases/embeddedcluster.replicated.com_kubernetesinstallations.yaml
Outdated
Show resolved
Hide resolved
|
This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID. Online Installer: Airgap Installer (may take a few minutes before the airgap bundle is built): Happy debugging! |
| // If omitted, x86_64 is assumed for backward compatibility. | ||
| // +kubebuilder:validation:Optional | ||
| // +listType=set | ||
| Architecture []Architecture `json:"architecture,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the EC lint command be updated? i can easily see users misspelling this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, I can expand the EC lint command to cover this new field and make sure if anything is included with architecture, its either aarch64/amd64 or x86_64/amd64
|
|
||
| const ( | ||
| ArchitectureAarch64 Architecture = "aarch64" | ||
| ArchitectureX8664 Architecture = "x86_64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're using amd64 all over the codebases, let's just stick with that name instead of X8664? this is a product facing change though, so @ajp-io might have a different opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marccampbell weighed in early on to express that we should use x86_64 and aarch64 instead of amd64 and arm64
What this PR does / why we need it:
Adds optional architecture field to the embedded cluster config spec, where the expected values to be given are x86_64 and/or aarch64 where all the values included are supported architectures, and having both indicates the application can be on either architectures, not both. If the field is empty or not included, the intended behavior is to assume x86_64 as the architecture.
Does this PR require a test?
NONE
Does this PR require a release note?
Does this PR require documentation?
NONE