-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Add doCheckDisplayNameOrNull to jenkins core #9150
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. |
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.
Works great with the PR build and the downstream PR build.
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.
/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!
Is it okay for merge this? |
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
doCheckDisplayNameOrNull
fromAbstractProject
toTopLevelItemDescriptor
to allow reuse in pipeline.Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
N/A
Before the changes are marked as
ready-for-merge
:Maintainer checklist