-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: allow to customize config values on create new page #228
Conversation
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: Denis Golovin <dgolovin@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
This should be ready for a review. You need to use the main branch of desktop as this PR requires podman-desktop/podman-desktop#7659 to work. There is just one missing part which is we should hide the |
await presetChanged(provider, extensionContext, telemetryLogger); | ||
await createCrcVm(provider, extensionContext, telemetryLogger, logger); | ||
await saveConfig(params); |
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.
I would suggest to preserve previous behavior when this form creates and starts VM.
When opened it should show default values set in CRC Extension settings and let user to changes them and use as part of 'crc start' command without saving them in settings as default.
There could be a switch 'Save as default settings' if we want and saveConfig method should be called in create function before calling createCrcVm().
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.
I agree about showing the default values. The initial plan i had was this one but this is not currently supported by Desktop as far as i recall and I don't think a user is going to delete/create the VM several time so we should be good with what we have now as an initial release. This can be done in a follow-up PR. Even because it can be useful for other extensions and it is an improvement we should do on the Desktop side.
Regarding saving the values as default vs using them when starting the VM - i think there is a bit of confusion about how crc fit inside the Podman Desktop mechanisms.
In the create new
page we usually create the instance of something (for podman - a podman machine, for kind - a kind cluster) but we are not actually starting them (unless you set the option start now
). In CRC i tried to do the same but crc does not have the concept of a new instance. Everything is set up during the setup phase and then you have to start it.
So in the "creation" process what we do is just registering the kubernetes connection to Desktop with its custom lifecycle (start/stop/delete).
Now if you want to start the VM you can click on the start button from the resources page, but in that case you don't have any way to set the memory/cpu/disk, and i don't think that showing a quickpick is really user friendly. So it is easier to set them globally during the "create" process so that when the user start the VM, the properties are already saved.
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.
Yes +1, I don't think the settings for the "OpenShift Local" environments should be handle in a different way than what we are doing for podman machine. (For podman machine, we have "default values", but the user has no way to configure those in the preferences.)
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.
Sounds good to me then with nit. There should be markup paragraph before pull-secret file location configuration field explaining that if you have file with it you can configure it here, but it is optional. If you do not fill up file location for pull-secret file you are going to be requested to login using Red Hat SSO to pull pull-secret and store it in platform specific secure storage.
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.
@dgolovin done. Description updated. You can see it in the image on the first post above
FYI, the font for the pullsecret is not displayed (testing with podman-desktop/podman-desktop#7995) |
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.
LGTM, with nit (see my comment).
Signed-off-by: lstocchi <lstocchi@redhat.com>
Nice catch 👍 It is a problem on podman desktop tough, i'm gonna fix it there |
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.
Remarks:
- memory and disk are described in GiB but displayed in GB: that's incoherent
- can't we detect the pull secret has already been set and hide the parameter
- why can't we set the preset ?
Also I was in the state where CRC was installed but not setup (missing the bundle) and it is very hard from the resource page to leave that state but that may be not related to this PR
Signed-off-by: lstocchi <lstocchi@redhat.com>
When did you face the error with the memory value? When setting the slider or when hitting create? |
No create as the value set for memory is rejected by HyperV |
Signed-off-by: lstocchi <lstocchi@redhat.com>
I was not able to replicate it. Tried to set the memory to 4.5 -5 and the vm was started without any problem.
After some investigation it seems that the problem is not that easy to solve. Basically the |
I don't understand fully because when I create a Kind cluster (I suppose this is also KubernetesProviderConnectionFactory), I have this screen: So what is the difference ? |
Have you tried with the latest commit? |
Not yet :) |
Seems it's working better than expected: So if I set 4.5GB I have these settings:
And starting DEBU CRC version: 2.37.1+36d451 DEBU Found crc-background-launcher.exe version 0.0.0.1 DEBU Copying 'C:\Users\Jeff.crc\cache\crc_microshift_hyperv_4.15.14_amd64\podman.exe' to 'C:\Users\Jeff.crc\bin\podman\podman.exe'
Error creating machine: Error in driver during machine creation: exit status 1
|
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.
I still have an issue with VM start failure not reported but it is not related to this PR so I will open a follow up issue
Yeah, I'll investigate it as well. I'm worried that when using the minimum amount of memory allowed, the conversion is not optimal and the resources given to micro shift are not enough. I'll give this a look when I have some free time |
This PR updates the extension to show the properties that can be customized when starting the VM in the
create new
page (memory, cpus, disksize and pull-secret-file).They have been removed from the settings page to be in line with the other podman desktop extensions.
So, when clicking on create, it updates the config values of cpus/memory/disksize/pull-secret and if the start now option is true (by default) the VM is also started. Otherwise it just updates the config and register the provider to start/stop/delete the VM into Desktop.
Right now there is no way to customize/update dynamically the minimum value of a config so i had to create 2 entries for both the memory and cpus (one for openshift and one for minishift). They are displayed accordingly to the preset value.
This requires podman-desktop/podman-desktop#7659