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

Add doCheckDisplayNameOrNull to jenkins core #9150

Conversation

krisstern
Copy link
Member

@krisstern krisstern commented Apr 9, 2024

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

  • Move doCheckDisplayNameOrNull from AbstractProject to TopLevelItemDescriptor 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

@daniel-beck
Copy link
Member

daniel-beck commented Apr 9, 2024

This needs an explanation, because the code location is unexpected.

Why did you choose to do this rather than pull up

@Restricted(DoNotUse.class)
public FormValidation doCheckDisplayNameOrNull(@AncestorInPath AbstractProject project, @QueryParameter String value) {
return Jenkins.get().doCheckDisplayName(value, project.getName());
}
into Job? Seems like that would automatically take care of the plugin.

[workflow-job] Un-inlining WorkflowJob/configure-entries.jelly

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>.
Copy link
Member

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?

Copy link
Member Author

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

@jglick
Copy link
Member

jglick commented Apr 9, 2024

Does not seem to align with

an AbstractItemDescriptor interface with a default implementation for that doCheckDisplayNameOrNull

by @mawinter69 in jenkinsci/workflow-job-plugin#411 (comment)

@mawinter69
Copy link
Contributor

I would expect this to be in TopLevelItemDescriptor or a dedicated interface that plugins implement, Job is not sufficient as we also need to check for duplicates with Folders.

@krisstern
Copy link
Member Author

This needs an explanation, because the code location is unexpected.

Why did you choose to do this rather than pull up

@Restricted(DoNotUse.class)
public FormValidation doCheckDisplayNameOrNull(@AncestorInPath AbstractProject project, @QueryParameter String value) {
return Jenkins.get().doCheckDisplayName(value, project.getName());
}

into Job? Seems like that would automatically take care of the plugin.

[workflow-job] Un-inlining WorkflowJob/configure-entries.jelly

Would you expect an average Jenkins admin to be able to understand this change and how it impacts them?

Sure, I just moved the code to Job. Actually I wasn't sure where to place the code originally, so I was making a wild guess. For more context please see jenkinsci/workflow-job-plugin#411.

@krisstern
Copy link
Member Author

TopLevelItemDescriptor

No problem, let me move it again.

@krisstern krisstern changed the title Feat/add do check display name or null to jenkins core Feat: Add doCheckDisplayNameOrNull to jenkins core Apr 9, 2024
@daniel-beck daniel-beck added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Apr 9, 2024
@krisstern krisstern force-pushed the feat/add-doCheckDisplayNameOrNull-to-jenkins-core branch from 8a63360 to 3aa9d08 Compare April 9, 2024 17:26
@mawinter69
Copy link
Contributor

I just noticed that the doCheckDisplayName method in Jenkins is broken (unrelated to this addition). When you check the displayname of something that is inside a folder it will actually check against items in root.

@MarkEWaite MarkEWaite added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Apr 9, 2024
@mawinter69
Copy link
Contributor

created https://issues.jenkins.io/browse/JENKINS-72988 for the wrong check

@daniel-beck
Copy link
Member

Looks reasonable now. Please file a downstream PR once the incremental is deployed, demonstrating this works as expected, then check

For new APIs and extension points, there is a link to at least one consumer.

Copy link
Contributor

@mawinter69 mawinter69 left a 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

@mawinter69
Copy link
Contributor

mawinter69 commented Apr 9, 2024

Looks reasonable now. Please file a downstream PR once the incremental is deployed, demonstrating this works as expected, then check

For new APIs and extension points, there is a link to at least one consumer.

we would need PRs in folder and matrix project plugins that remove the implementation there

@krisstern krisstern force-pushed the feat/add-doCheckDisplayNameOrNull-to-jenkins-core branch from 4be5bd0 to e5c9e75 Compare April 10, 2024 11:35
@krisstern
Copy link
Member Author

We should remove the implementation from AbstractProject as it is obsolete now

Done

@daniel-beck
Copy link
Member

Still needs downstream PRs.

Also, the proposed changelog entry needs work:

[workflow-job] Un-inlining WorkflowJob/configure-entries.jelly

Changelog audience is generally Jenkins users and administrators who we need to assume know nothing about Jenkins development.

@krisstern
Copy link
Member Author

Updated the "proposed changelog entries" in the description above

@krisstern
Copy link
Member Author

Still needs downstream PRs.

Also, the proposed changelog entry needs work:

[workflow-job] Un-inlining WorkflowJob/configure-entries.jelly

Changelog audience is generally Jenkins users and administrators who we need to assume know nothing about Jenkins development.

Downstream changes made in jenkinsci/workflow-job-plugin#411.

Copy link
Member

@daniel-beck daniel-beck left a 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.

@timja timja added developer Changes which impact plugin developers and removed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Aug 26, 2024
Copy link
Member

@timja timja left a 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!

@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 Aug 26, 2024
@krisstern
Copy link
Member Author

Is it okay for merge this?

@timja timja merged commit a075ffb into jenkinsci:master Aug 30, 2024
16 checks passed
@timja timja changed the title Feat: Add doCheckDisplayNameOrNull to jenkins core Add doCheckDisplayNameOrNull to jenkins core Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants