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
Feat: Add doCheckDisplayNameOrNull to jenkins core #9150
base: master
Are you sure you want to change the base?
Feat: Add doCheckDisplayNameOrNull to jenkins core #9150
Conversation
This needs an explanation, because the code location is unexpected. Why did you choose to do this rather than pull up jenkins/core/src/main/java/hudson/model/AbstractProject.java Lines 1943 to 1946 in b0baeba
Job ? Seems like that would automatically take care of the plugin.
Would you expect an average Jenkins admin to be able to understand this change and how it impacts them? |
import org.apache.commons.io.IOUtils; | ||
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
import org.kohsuke.stapler.AncestorInPath; | ||
import org.kohsuke.stapler.QueryParameter; | ||
|
||
/** | ||
* Dedicated class to handle the logic related to so-called <em>detached plugins</em>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this new method have to do with detached plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, I wasn't sure where to place the code at first actually
Does not seem to align with
by @mawinter69 in jenkinsci/workflow-job-plugin#411 (comment) |
I would expect this to be in |
Sure, I just moved the code to |
No problem, let me move it again. |
8a63360
to
3aa9d08
Compare
I just noticed that the |
created https://issues.jenkins.io/browse/JENKINS-72988 for the wrong check |
Looks reasonable now. Please file a downstream PR once the incremental is deployed, demonstrating this works as expected, then check
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the implementation from AbstractProject
as it is obsolete now
we would need PRs in folder and matrix project plugins that remove the implementation there |
4be5bd0
to
e5c9e75
Compare
Done |
Still needs downstream PRs. Also, the proposed changelog entry needs work:
Changelog audience is generally Jenkins users and administrators who we need to assume know nothing about Jenkins development. |
Updated the "proposed changelog entries" in the description above |
Downstream changes made in jenkinsci/workflow-job-plugin#411. |
See JENKINS-69916.
Testing done
Double checked no existing test has been caused to fail by runnig
mvn verfiy
. No test has been added since this will be done downstream.Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
N/A
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).