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-74741] Migrate from FromApply#applyResponse in ScriptlerBuilder.java #126

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

yaroslavafenkin
Copy link
Contributor

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

FormApply#applyResponse is deprecated in favor of CSP compatible FormApply#showNotification available since version 2.482 of Jenkins.

Testing done

Before the change
After the change

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@mtughan
Copy link
Contributor

mtughan commented Nov 18, 2024

Thanks @yaroslavafenkin. I won't be merging this quite yet as I'm working on a large update to the internals of the plugin to make it compatible with Jenkins 2.479+ and Java 17, but I will bring this in shortly after.

One other question: this PR is currently marked as a draft. Is that intentional?

@yaroslavafenkin
Copy link
Contributor Author

Thanks @yaroslavafenkin. I won't be merging this quite yet as I'm working on a large update to the internals of the plugin to make it compatible with Jenkins 2.479+ and Java 17, but I will bring this in shortly after.

One other question: this PR is currently marked as a draft. Is that intentional?

Hi.
Yes, PR is intentionally marked as a draft for now. When it comes to the CSP code it seems fine. I'm not sure if I handled the bump of the core version correctly (along with the Jakarta thing) as unit tests are failing. For that reason I'm not marking this as ready.
I think this can wait for your large update. I can make the adjustments to this PR after you're done.

@basil basil marked this pull request as ready for review November 19, 2024 18:33
@basil
Copy link
Member

basil commented Nov 19, 2024

I'm not sure if I handled the bump of the core version correctly (along with the Jakarta thing) as unit tests are failing. For that reason I'm not marking this as ready.

Fixed in commit a1b0467

@basil
Copy link
Member

basil commented Nov 20, 2024

I won't be merging this quite yet as I'm working on a large update to the internals of the plugin to make it compatible with Jenkins 2.479+ and Java 17, but I will bring this in shortly after.

This PR has a fully passing CI build, so is there any reason to merge this PR after the update you are working on rather than before? True, this PR updates the baseline to a weekly release, which is rare (in this case, a CSP method has not been backported to LTS yet), but if there is a desire to deliver any updates to users of LTS lines, a backport branch can be created as in https://gist.github.com/jglick/86a30894446ed38f918050c1180483e2.

@mtughan
Copy link
Contributor

mtughan commented Nov 22, 2024

@yaroslavafenkin, anything else stand out to you or is this good to merge?

@yaroslavafenkin
Copy link
Contributor Author

@yaroslavafenkin, anything else stand out to you or is this good to merge?

LGTM

@mtughan mtughan merged commit d01d180 into jenkinsci:main Nov 22, 2024
17 checks passed
@mtughan
Copy link
Contributor

mtughan commented Nov 22, 2024

Thanks for contributing this!

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

Successfully merging this pull request may close these issues.

3 participants