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-69032] Plugin manager update site URL can be saved empty #6886

Merged
merged 15 commits into from
Oct 3, 2022

Conversation

benebsiny
Copy link
Contributor

@benebsiny benebsiny commented Jul 20, 2022

See JENKINS-69032.

update-site

Form validation and "Reset to default" button are implemented.

Proposed changelog entries

  • Add a "Reset to default" button to reset update site url to default.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@NotMyFault NotMyFault added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Jul 20, 2022
@NotMyFault NotMyFault requested a review from a team July 20, 2022 08:09
@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Jul 20, 2022
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Does the FormValidation validate the input though? It looks like you'd be able to hit the submission button still, even if the field is empty.

@benebsiny
Copy link
Contributor Author

@NotMyFault The FormValidation can only shows the error message at most, just like some input fields in the Configure System page. In my opinion, if a user change the update site url to empty accidently, he/she can reset back easily by clicking the "Reset to default" button.

@daniel-beck
Copy link
Member

Given that accepting user submissions even if they're nonsensical is a fairly common pattern in Jenkins, validation + reset button doesn't seem like a bad solution.

@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jul 20, 2022
@NotMyFault NotMyFault removed the request for review from a team July 20, 2022 22:52
@NotMyFault NotMyFault requested a review from a team July 20, 2022 22:56
@daniel-beck
Copy link
Member

It might also be useful to review the error that appears when you check for updates from an empty URL update site to make sure that explains the configuration is screwed up and should be changed. Not necessary if you don't feel like adding that though.

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
@NotMyFault NotMyFault self-requested a review July 21, 2022 19:53
@NotMyFault NotMyFault added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Aug 12, 2022
Co-authored-by: James Nord <jtnord@users.noreply.github.com>
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Looks ok to me. Chinese translation passed the google translate check

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

LGTM - left a few untested suggestions.

// Connect to the URL
HttpURLConnection conn = (HttpURLConnection) ProxyConfiguration.open(url);
conn.setRequestMethod("HEAD");
conn.setConnectTimeout(5000);
Copy link
Member

Choose a reason for hiding this comment

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

should this also have a read timeout?

Suggested change
conn.setConnectTimeout(5000);
conn.setReadTimeout(1000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will 1 sec too short? How about 5 sec which is same as connect timeout?

core/src/main/java/hudson/PluginManager.java Show resolved Hide resolved
core/src/main/java/hudson/PluginManager.java Show resolved Hide resolved
@NotMyFault NotMyFault removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Sep 19, 2022
Copy link
Member

@NotMyFault NotMyFault 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 addressing the outstanding concerns, looks promising so far.

@NotMyFault NotMyFault requested a review from a team September 29, 2022 21:58
@timja
Copy link
Member

timja commented Oct 1, 2022

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 1, 2022
@timja timja merged commit 0cb9e02 into jenkinsci:master Oct 3, 2022
@jtnord
Copy link
Member

jtnord commented Dec 9, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants