Skip to content

Conversation

@atiratree
Copy link
Member

@atiratree atiratree commented Jan 9, 2020

  • unify and enhance template selections for validation and create requests
  • use GiB instead of GB for memory to allign with validations
  • show validation intervals

@yaacov @irosenzw

depends on

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 9, 2020
@openshift-ci-robot openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared labels Jan 9, 2020
@atiratree
Copy link
Member Author

keeps the validations messages and shows the options when all validation fail

inter

between

bellow

@atiratree
Copy link
Member Author

@rhrazdil fyi wizard will be using GiB after this change

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

why not exit here, empty validation will end up in line 121 anyway ... ? what am I missing here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This just adds default validation for positive integer (not empty one), so there is no chance to insert negative values or 0

Copy link
Member

Choose a reason for hiding this comment

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

👍 don't we have some other mechanism to enforce that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

we do, but just on the UI side in our component. IMO it will not hurt to have it also here

Copy link
Contributor

@irosenzw irosenzw Jan 14, 2020

Choose a reason for hiding this comment

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

@suomiy Why returning the validations from just one template? why not all of them ?
I think lines 38 and 41 should be switched. Am I wrong ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored the code to make the intent more clear and added a comment. Can you please check the code if it makes sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider the option that a validation is optional - a.k.a has the key justWarning.
optional validation shouldn't be combined and enforced.

Copy link
Member Author

Choose a reason for hiding this comment

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

right! added the support for justWarning

irosenzw added a commit to irosenzw/console that referenced this pull request Jan 16, 2020
Fixes: CNV-2914

 - Enforce the validations from common and user templates
   so the user can choose only from the allowed values in the Disk-modal
   at the storage tab.
 - Add the validations to the store.
 - Implement enum validation methods.

Depends on: openshift#3909

Signed-off-by: Ido Rosenzwig irosenzw@redhat.com
@atiratree atiratree force-pushed the kubevirt.memoryValidation branch from f9c09da to a3df828 Compare January 16, 2020 20:38
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020
@atiratree atiratree force-pushed the kubevirt.memoryValidation branch from a3df828 to ccd10ca Compare January 16, 2020 20:45
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 16, 2020
@atiratree atiratree force-pushed the kubevirt.memoryValidation branch from ccd10ca to aa575ff Compare January 16, 2020 22:41
@atiratree
Copy link
Member Author

template with justWarning validation found:

mem

@atiratree atiratree force-pushed the kubevirt.memoryValidation branch 3 times, most recently from b2473ca to a87aaa6 Compare January 20, 2020 16:13
@atiratree
Copy link
Member Author

rebased

Copy link
Contributor

@irosenzw irosenzw left a comment

Choose a reason for hiding this comment

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

/lgtm

@irosenzw
Copy link
Contributor

/lgtm

@atiratree
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@atiratree
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

- unify and enhance template selections for validation and create requests
- use GiB instead of GB for memory to allign with validations
- show validation intervals
@atiratree atiratree force-pushed the kubevirt.memoryValidation branch from a87aaa6 to 1178a24 Compare January 21, 2020 10:03
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020
@atiratree
Copy link
Member Author

fixed interval description: now uses at least and at most

aa

@irosenzw
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irosenzw, suomiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@atiratree
Copy link
Member Author

/retest

@spadgett
Copy link
Member

/hold
the merge queue is blocked

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3a0cce3 into openshift:master Jan 22, 2020
irosenzw added a commit to irosenzw/console that referenced this pull request Jan 23, 2020
Fixes: CNV-2914

Enforce the validations from common and user templates
so the user can choose only from the allowed values in the Disk-modal
at the storage tab.

Depends on: openshift#3909

Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants