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

Do not fail if optional plugin cannot be retrieved #317

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

Do not fail if optional plugin cannot be retrieved #317

wants to merge 3 commits into from

Conversation

PierreBtz
Copy link

@PierreBtz PierreBtz commented Apr 6, 2021 •
edited by timja

…f an optional plugin cannot be retrieved

Swallow the PluginNotFoundException if the dependency is optional.

Opening as a WIP PR, I'm not happy to have to do this this way but I'm not exactly sure how the optional dependencies are treated in the project, not sure why they are not skipped if they are causing trouble. Happy to discuss further to enhance the solution.
Once we agree on a solution I'll add tests.

Fixes #316

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master 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

@PierreBtz
Fixes jenkinsci#316: When resolving dependencies using manifest, do n…
ba3fda0 
…ot fail if an optional plugin cannot be retrieved
@PierreBtz
Fixes jenkinsci#316: When resolving dependencies using manifest, do n…
6407cf3 
…ot fail if an optional plugin cannot be retrieved
@PierreBtz
Copy link
Author

@timja I followed your advice and modify the getLatestPluginVersion directly. Not sure if this class is providing a public API for downstream projects so I kept the old method, let me know if it's ok to break compatibility and delete it.

@PierreBtz PierreBtz marked this pull request as ready for review April 7, 2021 14:31
@timja
Copy link
Member

timja commented Apr 7, 2021

@timja I followed your advice and modify the getLatestPluginVersion directly. Not sure if this class is providing a public API for downstream projects so I kept the old method, let me know if it's ok to break compatibility and delete it.

🤷 no real policy on it here. I'm not aware of any tools embedding this that would update without getting a compile time error, but you can just deprecate it and can remove them in a major version sweep at some point.

@timja timja added the bug Something isn't working label Apr 7, 2021
@timja timja changed the title Fixes #316: When resolving dependencies using manifest, do not fail i… Do not fail if optional plugin cannot be retrieved Apr 7, 2021
oleg-nenashev
oleg-nenashev requested changes Apr 7, 2021
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

The assumption about optional plugins is not fully correct. We do care about the version if there is a conflict.

E.g. we may have plugins A depending on C:1.2:optional and B depending on C:1.1:mandatory. In this case dependency metadata download failure for A will lead to the installation of C:11 which might be binary incompatible with A and lead to the Jenkins startup failure.

I am fine with more relaxed rules for optional dependencies, but this one does not seem to be the best approach when enabled by default.

@timja
Copy link
Member

timja commented Apr 7, 2021

I am fine with more relaxed rules for optional dependencies, but this one does not seem to be the best approach when enabled by default.

no options please, the current ones are confusing enough.

if it's optional we can ignore it, not sure if you can set the version number to be 'optional'?

@PierreBtz
Copy link
Author

@oleg-nenashev good point but I'm not sure how the case you mention could happen given that if C is not proposed by the UC you will never be able to resolve any version, right? So in this use case, the CLI will fail anyway when resolving the dependencies of B.

no options please, the current ones are confusing enough.

+1 :)

@PierreBtz
Copy link
Author

if it's optional we can ignore it, not sure if you can set the version number to be 'optional'?

Not sure I didn't read the whole codeline (yet :)) but I assume that if I return "latest", and there are two different versions to resolve at some point, the non latest one would win... so the proposed solution would be equivalent to specifying an optional version.

@PierreBtz
Copy link
Author

@oleg-nenashev happy to improve this PR to get it over the finish line, would you mind giving me an example of what this PR would break, I'm not sure I can find one (see my previous answer for my reasoning: #317 (comment)).

@oleg-nenashev oleg-nenashev self-requested a review April 16, 2021 10:35
@timja
Copy link
Member

timja commented Jun 25, 2021

@oleg-nenashev ^^

@oleg-nenashev oleg-nenashev self-assigned this Oct 17, 2021
@oleg-nenashev
Copy link
Member

Sorry, I missed it

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

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

Assignees

@oleg-nenashev oleg-nenashev

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

Successfully merging this pull request may close these issues.

When resolving dependencies using manifest, do not fail if an optional plugin cannot be retrieved
3 participants
@PierreBtz @timja @oleg-nenashev