plugin-installation-manager-tool icon indicating copy to clipboard operation
plugin-installation-manager-tool copied to clipboard

Do not fail if optional plugin cannot be retrieved

Open PierreBtz opened this issue 4 years ago • 8 comments

…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

  • [x] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • [x] Ensure that the pull request title represents the desired changelog entry
  • [x] Please describe what you did
  • [x] 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 avatar Apr 06 '21 15:04 PierreBtz

@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 avatar Apr 07 '21 14:04 PierreBtz

@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 avatar Apr 07 '21 14:04 timja

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'?

timja avatar Apr 07 '21 15:04 timja

@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 avatar Apr 07 '21 15:04 PierreBtz

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 avatar Apr 07 '21 15:04 PierreBtz

@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: https://github.com/jenkinsci/plugin-installation-manager-tool/pull/317#issuecomment-815014197).

PierreBtz avatar Apr 15 '21 12:04 PierreBtz

@oleg-nenashev ^^

timja avatar Jun 25 '21 06:06 timja

Sorry, I missed it

oleg-nenashev avatar Oct 17 '21 18:10 oleg-nenashev