Skip to content
This repository has been archived by the owner on Jun 2, 2023. It is now read-only.

Notification does not handle error when value below minimum/default #120

Open
Tracked by #2970
adrianriobo opened this issue Jan 28, 2022 · 11 comments
Open
Tracked by #2970

Comments

@adrianriobo
Copy link

adrianriobo commented Jan 28, 2022

Steps

  1. Run onboarding with podman preset
$ crc config view
preset podman

$ crc config
... memory (default: 2GB)
  1. Open configuration window change preset to openshift and save
$ crc config view
preset openshift

$ crc config
... memory (default: 9GB)
  1. Got a notification

The preset is stored, but the memory assignment is not persisted as the set value is below the default of the preset.

Expected

Configuration should be updated

Actual

Configuration seems persisted, as if I open the configuration window again I can see openshift preset as selected (and Openshift tab is now accessible) but when I did the step 2 I got this notification which seems confusing

You can see the second time I opened the configuration window with the openshift preset selected but the notification from previous change and save

image

@anjannath
Copy link
Member

configuration does get updated, as in can be verified from crc config view, for showing correct notifications, we need to first resolve #103

@gbraad
Copy link
Collaborator

gbraad commented Jan 28, 2022

Steps are incomplete. The configuration is not saved as the memory assignment is below the threshold/default.

@adrianriobo
Copy link
Author

🤔 so I need to updated them manually?

@gbraad
Copy link
Collaborator

gbraad commented Jan 28, 2022

so I need to updated them manually?

I am only saying your description is incomplete. Details like crc config and crc config view would have helped.

@adrianriobo
Copy link
Author

No, my description is exactly describing what I did.

I did not notice the values for memory, but to be fair I just assume that should be automate updated?

Also now I try and seeing this:

image

so memory box seems too short for bigger values (at least when increasing from podman preset). Do I create a new one for this?

@adrianriobo
Copy link
Author

The configuration is not saved as the memory assignment is below the threshold/default

Yeah you are right with valid amounts it is saved

@gbraad
Copy link
Collaborator

gbraad commented Jan 28, 2022

Defaults are not returned from the backend, or appropriately handled and would therefore be considered 'new functionality'. This is a known issue that has been an ongoing discussion.

We could try to see if the actual 'failing' configuration gets returned. Can you verify if the other values where stored?

@gbraad
Copy link
Collaborator

gbraad commented Jan 28, 2022

The error returns:

 body: "Value '2084' for configuration property 'memory' is invalid, reason: requires memory in MiB >= 9216\n"

We do not show this in the error returned, but it would be better to have validation, for this crc-org/crc#2869 is needed.

@gbraad gbraad changed the title Wrong notification information on save preset change Notification does not handle error when value below minimum/default Jan 28, 2022
@gbraad
Copy link
Collaborator

gbraad commented Jan 28, 2022

The problem with the error returned is that the body can not easily be parsed. Especially with multiple issues:

  body: "Value '6144' for configuration property 'memory' is invalid, reason: requires memory in MiB >= 9216\n" +
    "Value '19' for configuration property 'disk-size' is invalid, reason: requires disk size in GiB >= 31\n"

@adrianriobo
Copy link
Author

adrianriobo commented Jan 28, 2022

Defaults are not returned from the backend, or appropriately handled and would therefore be considered 'new functionality'

I guess that would be the best approach...to be able to automate update values accordingly with preset selection.

On the other hand, I guess that value validation error can not be show on the notification message?. As at the moment you see it...but as I said from a user perspective you have no clue what is going on specially because the openshift preset is shown as selected..

@gbraad
Copy link
Collaborator

gbraad commented Jan 28, 2022

On the other hand, I guess that value validation error can not be show on the notification message?.

Unfortunately not easy. They are formatted as if they are written on the command line.

I probably will change the notification first to be : "Error occurred saving the configuration" or so. And for the time being use the component to deal with the basic validation: crc-org/crc-react-components#33. This makes the component prevent these saves from happening... so if they occur it is most likely something else.

This is not recommended, but I have seen this discussion going back and forth quite a while now. The component would be the quickest way to a working scenario. Though I will leave the option to provide them remotely.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants