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-69916] Un-inline WorkflowJob/configure-entries.jelly #411

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

krisstern
Copy link
Member

@krisstern krisstern commented Feb 3, 2024

Description

Testing done. Tried the functionality which appears intact after the code modifications, except for the original testing steps outlined in https://issues.jenkins.io/browse/JENKINS-69916. The checkURL validation is now completely gone. This work follows the upstream changes in jenkinsci/jenkins#8866, especially the ones in core/src/main/java/hudson/model/AbstractProject.java. The only requirement for this patch to work is that we will need the minimum Jenkins version requirement to set at 2.443.

JIRA Issue

https://issues.jenkins.io/browse/JENKINS-69916

Relevant Upstream Change(s)

jenkinsci/jenkins#8866

Testing Steps

Similar to the ones described in JIRA ticket, except the checkDisplayName validation is gone.

c.c. @Kevin-CB

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
    Options
  2. Ensure that the pull request title represents the desired changelog entry
    Options
  3. Please describe what you did
    Options
  4. Link to relevant issues in GitHub or Jira
    Options
  5. Link to relevant pull requests, esp. upstream and downstream changes
    Options
  6. Ensure you have provided tests - that demonstrates feature works or fixes the issue
    Options

@krisstern krisstern requested a review from a team as a code owner February 3, 2024 08:20
@krisstern krisstern changed the title Feat: Un-inline WorkflowJob/configure-entries.jelly [JENKINS-69916] Un-inline WorkflowJob/configure-entries.jelly Feb 3, 2024
@mawinter69
Copy link

mawinter69 commented Feb 3, 2024

You can't depend on jenkinsci/jenkins#8866 because this changes AbstractProject and WorkflowJob is not inheriting from it. So no need to change the jenkins version. You would need to also implement the doCheckDisplayNameOrNull method.

Given that we have this now in MatrixProject (open PR jenkinsci/matrix-project-plugin#130), core (the mentioned change), folder plugin (https://github.com/jenkinsci/cloudbees-folder-plugin/blob/233481b4bb4accfe8582ad2f5c922168022af31d/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolderDescriptor.java#L138) and here maybe we can have an AbstractItemDescriptor interface with a default implementation for that doCheckDisplayNameOrNull and have all 4 places implement that interface

@krisstern
Copy link
Member Author

Hi @mawinter69 I have tried my best to apply the suggesions from your review. But I might have missed some important details though. If that if the case please let me know. Thanks!

@mawinter69
Copy link

Hi @mawinter69 I have tried my best to apply the suggesions from your review. But I might have missed some important details though. If that if the case please let me know. Thanks!

Sorry seems my wording was not precise enough. The thing with the AbstractItemDescriptor is something that would need to be implemented in Jenkins core, not in a plugin. This would allow the 4 places that now all have duplicated code for the doCheckDisplayNameOrNull method now to just add the dependency on that interface.

So for now you will need to add this method to the Descriptor of WorkflowJob, not WorkflowJob directly

@jglick
Copy link
Member

jglick commented Feb 6, 2024

something that would need to be implemented in Jenkins core, not in a plugin

So do so in Jenkins core, and set the jenkins.version here appropriately to pick up the new API.

@krisstern
Copy link
Member Author

Let me follow up on this shortly

@krisstern
Copy link
Member Author

Hi @mawinter69 @jglick,
I have tried my best in jenkinsci/jenkins#9150.

But if I have made any mistakes, please point these out to me so I can accommodate. Thanks!

@krisstern
Copy link
Member Author

Working on the upstream PR pending review(s): jenkinsci/jenkins#9150

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.

With the core change, should be good enough just deleting the checkUrl?

Should increase the core dependency to the snapshot (and then the release once out).

@krisstern krisstern requested a review from daniel-beck May 2, 2024 14:09
@krisstern
Copy link
Member Author

Hi @daniel-beck I have just made the changes as suggested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants