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

Do not fail if optional plugin cannot be retrieved #317

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

Conversation

PierreBtz
Copy link

@PierreBtz PierreBtz commented Apr 6, 2021

…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

…ot fail if an optional plugin cannot be retrieved
…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
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
Labels
bug Something isn't working
Projects
None yet
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