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.

Sign up for GitHub

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

Jump to bottom

Use JENKINS_UC_DOWNLOAD_URL if provided #416

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

Use JENKINS_UC_DOWNLOAD_URL if provided #416

wants to merge 2 commits into from

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

timja
timja approved these changes Mar 10, 2022
View reviewed changes
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
Merged
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
@cronik cronik mentioned this pull request Jun 20, 2022
Open
julien-demaziere
julien-demaziere approved these changes Apr 17, 2023
View reviewed changes
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timja timja timja approved these changes

@julien-demaziere julien-demaziere julien-demaziere approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JENKINS_UC_DOWNLOAD_URL usage logic incorrect
3 participants
@nhojpatrick @timja @julien-demaziere