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

feat: Adds option to pull arm64/amd64 examples #1196

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Jan 16, 2025

feat: Adds option to pull arm64/amd64 examples

What does this PR do?

  • Adds the ability to choose what initial arm64 or amd64 image example
    you want
  • Defaults to native architecture when initially loading
  • Pulls the architecture correctly

Screenshot / video of UI

Screen.Recording.2025-01-16.at.2.14.53.PM.mov

What issues does this PR fix or reference?

Closes #1154

How to test this PR?

  1. Go to examples page
  2. Select from the drop down the arch you want
  3. Pull the image & build

@cdrage cdrage requested a review from a team as a code owner January 16, 2025 20:20
@cdrage cdrage requested review from benoitf, dgolovin and axel7083 and removed request for a team January 16, 2025 20:20
@cdrage cdrage force-pushed the pull-different-archs branch 2 times, most recently from 7869519 to 5c9bb3f Compare January 16, 2025 20:39
Copy link
Contributor

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

Need to update the copyright in the .ts files (2024-2025).

Instead of adding isArm() to the API, could we do getArch()? Seems a bit cleaner for future use. (or we could use os.arch() in the frontend?)

@cdrage cdrage force-pushed the pull-different-archs branch from c11df84 to 46b451e Compare January 20, 2025 14:04
@cdrage
Copy link
Contributor Author

cdrage commented Jan 20, 2025

Need to update the copyright in the .ts files (2024-2025).

Instead of adding isArm() to the API, could we do getArch()? Seems a bit cleaner for future use. (or we could use os.arch() in the frontend?)

We don't have getArch in the PD API, so instead I've added isArm and used the already-added isX86.

At the moment the only other arch that we could possibly support is x390 but I'm unsure what the status of that is.

I've updated the code / tests!

@cdrage cdrage requested a review from deboer-tim January 20, 2025 14:05
@deboer-tim
Copy link
Contributor

Need to update the copyright in the .ts files (2024-2025).

Thanks for updating!

Instead of adding isArm() to the API, could we do getArch()? Seems a bit cleaner for future use. (or we could use os.arch() in the frontend?)

We don't have getArch in the PD API, so instead I've added isArm and used the already-added isX86.

At the moment the only other arch that we could possibly support is x390 but I'm unsure what the status of that is.

But why not just expose a getArch() function directly, that just returns the value from os.arch [*]? It would be one less function and avoid potentially having to add something like isS390() in the future.

[*] I'm also finding it a bit odd that the UI uses isX86() to decide to default to the 'amd64' arch. One option would be to use the values from os.arch() consistently, but I think it would be cleaner for bootc.ts to include something like export type Architecture = 'arm64' | 'amd64'

Then the API could export one getArch(): Architecture function, and both backend & shared pullImage() functions could have some type safety with arch?: Architecture instead of arch?: string. Sound ok?

@cdrage
Copy link
Contributor Author

cdrage commented Jan 21, 2025

Need to update the copyright in the .ts files (2024-2025).

Thanks for updating!

Instead of adding isArm() to the API, could we do getArch()? Seems a bit cleaner for future use. (or we could use os.arch() in the frontend?)

We don't have getArch in the PD API, so instead I've added isArm and used the already-added isX86.
At the moment the only other arch that we could possibly support is x390 but I'm unsure what the status of that is.

But why not just expose a getArch() function directly, that just returns the value from os.arch [*]? It would be one less function and avoid potentially having to add something like isS390() in the future.

[*] I'm also finding it a bit odd that the UI uses isX86() to decide to default to the 'amd64' arch. One option would be to use the values from os.arch() consistently, but I think it would be cleaner for bootc.ts to include something like export type Architecture = 'arm64' | 'amd64'

Then the API could export one getArch(): Architecture function, and both backend & shared pullImage() functions could have some type safety with arch?: Architecture instead of arch?: string. Sound ok?

Sounds good! I'll update it for that. For some reason I thought you meant adding "getArch" upstream to PD, but I see we don't have anything up there.

I'll add the getArch function and you are right, instead we can just do if / switch statements to determine if something is arm64 / amd64, etc.

### What does this PR do?

* Adds the ability to choose what initial arm64 or amd64 image example
  you want
* Defaults to native architecture when initially loading
* Pulls the architecture correctly

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes podman-desktop#1154

### How to test this PR?

<!-- Please explain steps to reproduce -->

1. Go to examples page
2. Select from the drop down the arch you want
3. Pull the image & build

Signed-off-by: Charlie Drage <[email protected]>
Signed-off-by: Charlie Drage <[email protected]>
Signed-off-by: Charlie Drage <[email protected]>
@cdrage cdrage force-pushed the pull-different-archs branch from 46b451e to 0fb5e23 Compare January 22, 2025 16:26
@cdrage
Copy link
Contributor Author

cdrage commented Jan 22, 2025

Updated to use getArch instead (in a separate commit) and ready for a re-review! @deboer-tim

Copy link
Contributor

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

Thanks Charlie, this is better!

As per my last comment the one thing I still don't like is how arch is just a string and the frontend needs to understand how to map that string between os.platform arch and container arch. I'd prefer to have BootcAPI getArch() and pullImage() use the same values (probably getArch() does some mapping) so that the UI can use the values directly and never needs code like this:

if (hostArch === 'x64') {
    defaultArch = 'amd64';

but I won't hold this up, we can take it as a follow-up.

@cdrage
Copy link
Contributor Author

cdrage commented Jan 23, 2025

Thanks Charlie, this is better!

As per my last comment the one thing I still don't like is how arch is just a string and the frontend needs to understand how to map that string between os.platform arch and container arch. I'd prefer to have BootcAPI getArch() and pullImage() use the same values (probably getArch() does some mapping) so that the UI can use the values directly and never needs code like this:

if (hostArch === 'x64') {
    defaultArch = 'amd64';

but I won't hold this up, we can take it as a follow-up.

I agree. I'll follow this up in a separate PR.

@cdrage cdrage merged commit 750470b into podman-desktop:main Jan 23, 2025
6 checks passed
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.

Examples should have an option to pull AMD64 or ARM64 / default to host
2 participants