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

[JENKINS-49387][JENKINS-49520] - Validation errors #3292

Merged
merged 2 commits into from Feb 18, 2018

Conversation

@ksenia-nenasheva
Copy link
Contributor

@ksenia-nenasheva ksenia-nenasheva commented Feb 14, 2018

See JENKINS-49387 and JENKINS-49520.

In the PR#3141 I broke validation for some fields.
This PR:

  • restores old rules for validation
  • applies new rules for fields which are mandatory

Proposed changelog entries

  • Entry 1: Prevent input validation errors in optional numeric form entries (regression in 2.105).
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

@ksenia-nenasheva
Copy link
Contributor Author

@ksenia-nenasheva ksenia-nenasheva commented Feb 14, 2018

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

LGTM

@oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Feb 14, 2018

@jenkinsci/code-reviewers this is a regression in 2.105 which impacts the current LTS baseline candidate (2.107). Would be great to get reviews so that we could merge it into the next weekly

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Looks good to me. I had to think more deeply than usual about the regular expressions, but as far as I can tell, they are correctly expressing the intended match.

"INPUT.positive-number" : function(e) {
registerRegexpValidator(e,/^(\d*[1-9]\d*|)$/,"Not a positive integer");
Copy link
Member

@daniel-beck daniel-beck Feb 14, 2018

Allowed to begin with leading zeroes why?

Copy link
Contributor

@MarkEWaite MarkEWaite Feb 14, 2018

I assumed it was because a leading 0 with at least one non-zero digit is a valid positive integer.

We don't accept octal constants or hex constants, so the leading zero is harmless (and thus should be allowed) - my opinion.

Copy link
Contributor Author

@ksenia-nenasheva ksenia-nenasheva Feb 14, 2018

I just restored the original behavior (see #3141).

Copy link
Member

@daniel-beck daniel-beck Feb 14, 2018

Right, but specifically \d* doesn't seem right. Accepting empty values seems fine to restore compatibility, but this? Unclear to me, why.

Copy link
Contributor Author

@ksenia-nenasheva ksenia-nenasheva Feb 15, 2018

I don't think it's critical. If you think it is necessary, I'll delete \d*.

Copy link
Contributor

@MarkEWaite MarkEWaite Feb 15, 2018

@daniel-beck the pattern as a whole does not accept empty values. It requires at least one digit from the set 1-9.

I am fine if we disallow a leading zero (thus allowing deletion of the initial \d*). The regular expression will need to have some portion which accepts zero or more digits, but if you feel that leading zeros are not needed, I think it is fine to remove the first \d*.

Copy link
Member

@daniel-beck daniel-beck Feb 15, 2018

the pattern as a whole does not accept empty values.

Not true. That's the whole reason there's parens around the digits.

^(\d*[1-9]\d*|)$
             ^^

Copy link
Contributor

@MarkEWaite MarkEWaite Feb 15, 2018

Ah, you're correct. Sorry about my poor parsing of the regular expression.

I think an empty field should be allowed so that a default value is assumed when none is provided.

I am indifferent whether leading zeros should be accepted.

@oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Feb 17, 2018

@daniel-beck @MarkEWaite What is your decision about that? Since it restores the original behavior, I would suggest merging it as is for backporting to 2.107.1 AND creating a follow-up ticket for your concerns

@MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Feb 17, 2018

I approve merging and backporting. I just saw an instance today of this bug. Once this is backported or available in a new weekly, I can test to confirm it is working as expected.

The problem case I saw was an intentionally empty field which was showing a red warning that it was not a positive integer.

@daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Feb 17, 2018

@oleg-nenashev Seems reasonable.

@oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Feb 17, 2018

@oleg-nenashev oleg-nenashev merged commit 5c8cc45 into jenkinsci:master Feb 18, 2018
1 check passed
olivergondza added a commit that referenced this issue Feb 28, 2018
* Fixes for validation

* Fix for an empty agent.nExecutors

(cherry picked from commit 5c8cc45)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants