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

Don't assume string params to Install are identifiers #2764

Merged
merged 2 commits into from
May 21, 2019

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 20, 2019

Problem

CmdLine's ckan install identifier=version syntax is broken:

$ ../CKAN/_build/ckan install Astrogator=v0.9.0
About to install...


Continue? [Y/n]

(The list of modules to install is missing.)

This also affects the ckan install -c localfile.ckan syntax:

$ ../CKAN/_build/ckan install -c KerbalAircraftExpansion-3-2.8.0.1.ckan
About to install...


Continue? [Y/n]

Which also breaks pull request validation:

https://ci.ksp-ckan.space/job/NetKAN/8294/console

Running ckan install -c built/KerbalAircraftExpansion-3-2.8.0.1.ckan
About to install...



Updating registry
Committing filesystem changes
Rescanning GameData
Done!

(Nothing gets installed.)

Noticed while checking validation of KSP-CKAN/NetKAN#7194.

Cause

This line from #2753 assumed that the list of strings passed to InstallList are pure identifiers:

InstallList(resolver.ModList().Where(m => modules.Contains(m.identifier)).ToList(), options, downloader);

But they can include the version as well, so comparing them to identifiers doesn't work as intended.

Changes

Now instead of checking that the module's identifier is present in the parameter list, we check that the resolver's ReasonFor is UserRequested. This has the intended effect of passing only CkanModules that were specified in the parameters (so the rest can be flagged as auto-installed), but does not break the identifier=version syntax.

And since it's kind of shocking that functionality this important broke and no one noticed, a new ModuleInstaller test is added for the identifier=version syntax, which fails on master and passes on this branch.

@HebaruSan HebaruSan added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Cmdline Issues affecting the command line Pull request Tests Issues affecting the internal tests labels May 20, 2019
@HebaruSan HebaruSan merged commit c579ac1 into KSP-CKAN:master May 21, 2019
@HebaruSan HebaruSan deleted the fix/inst-ver branch May 21, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants