Nitpicks from #3839 #3879
Nitpicks from #3839 #3879
Conversation
openshift-ci-robot
added
size/M
openshift-ci-robot
added
the
approved
|
/lgtm |
|
/test e2e-gcp-console |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
| getRelevantTemplates(commonTemplates, os, workloadProfile, flavor), | ||
| ); | ||
| const templates = getRelevantTemplates(commonTemplates, os, workloadProfile, flavor); | ||
| const validations = getValidationsFromTemplates(templates); |
There was a problem hiding this comment.
I think there is an issue with the multiple templates validations.
IMO the validations should be ignored when one template does not have any validation for min/max/between.
E.g. when workload profile is not selected, at this point it is not clear which single template will be selected. We should account for a possibility that each template can be selected and thus not limit the validations.
The exact validation for the min/max or none will be run again when the exact template is selected.
There was a problem hiding this comment.
IMO the validations should be ignored when one template does not have any validation for min/max/between.
Yes, if we have one template, current code ignore validation if no validations are found.
If we have multiple templates we ignore the templates with no validations, but do use the validations we find in other templates.
when workload profile is not selected, at this point it is not clear which single template will be selected. We should account for a possibility that each template can be selected and thus not limit the validations.
Correct, we where more restrictive, but not by much :-) in current code if we have multiple rules we take the most inclusive path possible.
For example:
if we have one template that require min=3 and another that require min=5, we will take all values that pass at least one of this rule, in this case, we will end up with a min=3 rule.
The exact validation for the min/max or none will be run again when the exact template is selected.
Yes, in current code, we recheck on each change of os, load or flavor.
There was a problem hiding this comment.
yes - it works for templates with diffent validations.
But still I think we should support this use case:
template-1 : no validations
template-2 : min 2G memory validation
-> user selects 2G because s/he thinks this is the lowest value possible
-> user selects all other fields
only template-1 is available now
any memory value is possible
-> user creates VM on false assumption with 2G memory
There was a problem hiding this comment.
hmm ...
nice, will look how to fix that !
| getRelevantTemplates(commonTemplates, os, workloadProfile, flavor), | ||
| ); | ||
| const templates = getRelevantTemplates(commonTemplates, os, workloadProfile, flavor); | ||
| const validations = getValidationsFromTemplates(templates); | ||
|
|
||
| // Return all the validations which are relevant for the field | ||
| return getFieldValidations(validations, IDToJsonPath[fieldId]); |
There was a problem hiding this comment.
also at https://github.com/openshift/console/pull/3879/files#diff-c3d6503935eeb5d51e6cca027270cb72R75
- is it possible for a template to have two standalone validations? One min and the other max?
- shouldn't we have a default validation.message if it is missing one?
- shouldn't we rather use custom messages where we can. E.g. memory validation
This VM requires more memory.is not much fitting for the wizard IMO. We should at least say what the minimal ammount is.
There was a problem hiding this comment.
is it possible for a template to have two standalone validations? One min and the other max?
I is grammatically possible, but is semantically nonsense, for example, one min and one max rule, will always pass validation whatever the values.
shouldn't we have a default validation.message if it is missing one?
Nice, fixing
shouldn't we rather use custom messages where we can. E.g. memory validation This VM requires more memory. is not much fitting for the wizard IMO. We should at least say what the minimal ammount is.
Makes sense to me too ... fixing.
There was a problem hiding this comment.
is it possible for a template to have two standalone validations? One min and the other max?
I is grammatically possible, but is semantically nonsense, for example, one
minand onemaxrule, will always pass validation whatever the values.
why nonsense? Min 1Gi and max 4Gi should check for both sides of the interval, no?
There was a problem hiding this comment.
why nonsense? Min 1Gi and max 4Gi should check for both sides of the interval, no?
In this case, any value is either above 1G or below 4G, so it's always true.
if users wants to check for between 1G and 4G they need to use the min/max rule.
There was a problem hiding this comment.
why is it or? Can you point me to the specification?
Wouldn't it make more sense to combine the validations with and?
I agree min/max in one validation is better, but should we expect that the users will always use it this way?
There was a problem hiding this comment.
why is it or?
The reasoning is:
We start from a none specific state where we match several templates ( for example all "windows" templates) in this state, all memory values matching at least one template is valid.
As we get more specific ( for example windows server ) we have less templates, but again, if at least one template is possible, the value is valid.
And when we have only one template, we will probable have only one rule ... ( min, max or min/max ), any combination of this rules is a miss configuration on the side of the template writer ( p.s. all the memory rules in common templates are min, and it makes sense too ... )
Can you point me to the specification?
multiple templates is outside the scope of the docs, IMHO we need to do what makes more sense.
Wouldn't it make more sense to combine the validations with and?
no, for example if windows-server need min=4G and windows-desktop need min=2G, in the state where we still don't know what template we end up with, we should allow the server or desktop, and recheck when we know what workload was chosen.
I agree min/max in one validation is better, but should we expect that the users will always use it this way?
yes,
a- min, max, and min/max have a defined meaning as one rule are defined, having two rules of min and max for same path is not defined.
b- all common template use one rule of min,max,min/max per path ( actually they use only min)
There was a problem hiding this comment.
why is it or?
The reasoning is:
We start from a none specific state where we match several templates ( for example all "windows" templates) in this state, all memory values matching at least one template is valid.
As we get more specific ( for example windows server ) we have less templates, but again, if at least one template is possible, the value is valid.
yup, true
And when we have only one template, we will probable have only one rule ... ( min, max or min/max ), any combination of this rules is a miss configuration on the side of the template writer ( p.s. all the memory rules in common templates are
min, and it makes sense too ... )
Can you point me to the specification?
multiple templates is outside the scope of the docs, IMHO we need to do what makes more sense.
I was talking about the miss configuration on the side of the template writer from the beginning. Do we have a specification for that?
One can write a user template with these two rules and think it is correct.
Wouldn't it make more sense to combine the validations with and?
no, for example if
windows-serverneedmin=4Gandwindows-desktopneedmin=2G, in the state where we still don't know what template we end up with, we should allow the server or desktop, and recheck when we know what workload was chosen.
not what I was talking about - just one single template with multiple validations
I agree min/max in one validation is better, but should we expect that the users will always use it this way?
yes,
a- min, max, and min/max have a defined meaning as one rule are defined, having two rules of min and max for same path is not defined.
I think we should support that if the behavior is not defined.
IMO it is more sane to behave like that. It is an array of validations so one would expect that all of them will execute.
b- all common template use one rule of min,max,min/max per path ( actually they use only min)
I know, this concerns just user templates now
| @@ -45,15 +37,24 @@ export const getTemplateValidations = ( | |||
| ? iUserTemplates.find((template) => iGetName(template) === userTemplateName) | |||
| : null; | |||
|
|
|||
| if (iUserTemplate) { | |||
| const validations = getValidationsFromTemplates(List([iUserTemplate])); | |||
| return getFieldValidations(validations, IDToJsonPath[fieldId]); | |||
There was a problem hiding this comment.
why are we using id for the lookup? We can just use VMSettingsField enum
There was a problem hiding this comment.
? it's a variable of type VMSettingsRenderableField , what am I missing here ?
There was a problem hiding this comment.
the type is correct, but the usage is not. Why are we calling getFieldId ?
There was a problem hiding this comment.
Nice !, didn't notice that , will fix.
| @@ -141,7 +141,7 @@ export const getRelevantTemplates = ( | |||
| workloadProfile: string, | |||
| flavor: string, | |||
| ) => { | |||
| if (!commonTemplates || commonTemplates.length === 0 || !os || !workloadProfile) { | |||
| if (!commonTemplates || commonTemplates.length === 0) { | |||
There was a problem hiding this comment.
(commonTemplates || []) bellow will do the same job
|
/lgtm cancel please relgtm if you plan to do other followup. IMO we could resolve the issues here |
yaacov
force-pushed
the
kubevirt-memeory-cleanups
branch
2 times, most recently
from
0847488
to
325b4ef
Compare
| let customMessage = validationResult.errorMsg; | ||
|
|
||
| if ('min' in validation && 'max' in validation) { | ||
| customMessage = `Memory value must be between ${validation.min / |
There was a problem hiding this comment.
isn't the "value" superfluous?
yaacov
force-pushed
the
kubevirt-memeory-cleanups
branch
from
325b4ef
to
e14d085
Compare
openshift-ci-robot
added
size/L
|
there are still some unresolved issue but let's go with this for now and resolve it in another followup /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irosenzw, suomiy, yaacov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-gcp-console |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
25 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Nitpicks left from #3839 + screenshots:
Screenshots:
