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

Use JENKINS_UC_DOWNLOAD_URL if provided #416

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

nhojpatrick
Copy link
Contributor

Fixes #415

The current code ignores JENKINS_UC_DOWNLOAD_URL if specified, as logic above will always result in something else being checked. Other if/else statements might also need to be changed to fix other issues with that logic.

  • 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

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.

I think ok

@timja
Copy link
Member

timja commented Mar 10, 2022

can you take a look at the tests?

@timja timja added the bug Something isn't working label Mar 10, 2022
@nhojpatrick
Copy link
Contributor Author

can you take a look at the tests?

Happy too, I've no time until weekend probably but will be looking at them then. Wanted to get it raised and review then fix tests.

@nhojpatrick
Copy link
Contributor Author

Fixing the same issue as highlighted here jenkinsci/docker#1299

@nhojpatrick nhojpatrick force-pushed the bugfix/JENKINS_UC_DOWNLOAD_URL branch from 75c592f to 8b6bedf Compare March 10, 2022 21:36
@nhojpatrick
Copy link
Contributor Author

I've found time.
To make testing easier I've added JUnit v5 and assertAll to all errors show.
I've updated so if you define JENKINS_UC_DOWNLOAD_URL it's used for everything, so a true global override.

@nhojpatrick nhojpatrick marked this pull request as draft March 11, 2022 08:07
@nhojpatrick nhojpatrick mentioned this pull request Mar 11, 2022
6 tasks
@nhojpatrick nhojpatrick force-pushed the bugfix/JENKINS_UC_DOWNLOAD_URL branch from 8b6bedf to a7d7873 Compare March 11, 2022 08:29
@nhojpatrick
Copy link
Contributor Author

Converting to draft and rebase from PR #417 so this PR easier to understand what has changed when using assertAll to improve dev/test cycles.

@nhojpatrick nhojpatrick force-pushed the bugfix/JENKINS_UC_DOWNLOAD_URL branch from a7d7873 to 4a31145 Compare March 11, 2022 17:14
@nhojpatrick nhojpatrick marked this pull request as ready for review March 11, 2022 20:30
Copy link

@julien-demaziere julien-demaziere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I ran the tests successfully on my PC with java 11

@sichapman
Copy link

Any chance of having this merged? Being able to use JENKINS_UC_DOWNLOAD_URL the way it is documented would be extremely useful

@timja timja merged commit b18bdd2 into jenkinsci:master Oct 9, 2024
16 checks passed
@MarkEWaite
Copy link
Contributor

Thanks very much @timja ! Will the jar file be published to the GitHub release automatically or is there some other step needed?

@timja
Copy link
Member

timja commented Oct 9, 2024

It will be I got distracted and didn't publish the release at the time, it should be attached here soon: https://github.com/jenkinsci/plugin-installation-manager-tool/releases/tag/2.13.1

@MarkEWaite
Copy link
Contributor

It will be I got distracted and didn't publish the release at the time, it should be attached here soon: https://github.com/jenkinsci/plugin-installation-manager-tool/releases/tag/2.13.1

Thanks. I could have published that GitHub release page myself and that would have solved it. I didn't even think of that. Sorry that I didn't do my part to help! Thanks for doing the release!

@cronik
Copy link
Contributor

cronik commented Oct 10, 2024

This change breaks all current usages where there is a plugin specific url defined in the plugin spec. Wouldn't it make more sense for the precedence to prefer the plugin url then use the download url if available?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JENKINS_UC_DOWNLOAD_URL usage logic incorrect
6 participants