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

Fix replicas limit throws "Replicas must be an integer" if any 10 digit number other than 1 is provided #2276

Merged

Conversation

gijohn
Copy link
Contributor

@gijohn gijohn commented Aug 6, 2019

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console labels Aug 6, 2019
@gijohn
Copy link
Contributor Author

gijohn commented Aug 6, 2019

/kind bug
/assign @christianvogt

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 6, 2019
maxpods: yup
.number()
.transform((cv) => (_.isNaN(cv) ? undefined : cv))
.integer('Max Pods must be an Integer.')
.test(isInteger('Max Pods must be an Integer.'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need a separate util isInteger here? What is the issue with using .integer from yup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In yup, to validate an Integer they have used the below validation code
value == null || (value | 0) == value
The maximum value it can validate successful is 2147483647 anything more than that, it fails.
`

let a = 2147483647;
a|0
<2147483647
(a+1)|0
<-2147483648
(a+2)|0
<-2147483647
`
This bug is fixed in jquense/yup#405 but they have not made any release with this fix. Myself and @christianvogt had a discussion and we conclude our own way of fixing until the formal release is made.

You can track the conversation in https://jira.coreos.com/browse/ODC-1225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the restriction of MAX_SAFE_INTEGER to max value is to enable correct validation.

`>let x = Number.MAX_SAFE_INTEGER + 1
<undefined

let y = Number.MAX_SAFE_INTEGER + 2
<undefined
x
<9007199254740992
y
<9007199254740992
x === y
<true`

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks.

@rohitkrai03
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2019
Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gijohn, rohitkrai03, vikram-raj

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gijohn
Copy link
Contributor Author

gijohn commented Aug 12, 2019

/test e2e-aws

@gijohn
Copy link
Contributor Author

gijohn commented Aug 12, 2019

/test e2e-aws-console-olm

@openshift-merge-robot openshift-merge-robot merged commit 3514b4e into openshift:master Aug 12, 2019
@gijohn gijohn deleted the fix-replica-limit-error branch August 13, 2019 09:06
@spadgett spadgett added this to the v4.2 milestone Aug 15, 2019
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/dev-console Related to dev-console kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants