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-57660] Addition of a doCheck validation method for invalid refSpec #826

Merged
merged 7 commits into from Jan 27, 2020

Conversation

rishabhBudhouliya
Copy link
Contributor

@rishabhBudhouliya rishabhBudhouliya commented Jan 26, 2020

JENKINS-57660

While adding a Git source in the Source Code Management, adding an invalid refSpec produces a confusing/complex stack trace upon applying/saving the configuration changes.

Upon discussion on this issue, a good way to inform the user about the invalid refSpec is to check the correctness of the values entered by the user on the fly (Done on the server-side). Hence, a doCheckRefspec method has been added to create a form validation logic.

Here is a screenshot of the UI result of the new method:
Screenshot 2020-01-26 at 6 10 06 PM

Note: The message has been changed to "Specification is invalid"

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks so much for this pull request! I've made comments on areas where I would like changes (add tests of more success cases and at least one failure case) and on areas where I need to investigate further (confirm that getAllRemoteConfigs does not connect to the remote git repository)

…rrect indentation

Co-Authored-By: Raihaan Shouhell <raihaanhimself@gmail.com>
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks again for the submission. Minor updates on my comments.

I clicked the "is documented" checkbox in the template because I don't think any change is needed to the README for this. You're providing user input validation that should have been provided before.

@MarkEWaite MarkEWaite added bugfix Fixes a bug - used by Release Drafter ShortTerm Short term Improvements labels Jan 26, 2020
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks for the additional tests. Very nice!

One open question on a possible unintentional behavior change from the current releases to this change.

@rishabhBudhouliya
Copy link
Contributor Author

@MarkEWaite I have added another test-case as requested, you can check it once and merge the PR.
Thank you for taking out the time to review it!

@MarkEWaite MarkEWaite merged commit 16ef0da into jenkinsci:master Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug - used by Release Drafter ShortTerm Short term Improvements
Projects
None yet
3 participants