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

Confirmation prompt for Cmdline upgrades #3204

Merged
merged 1 commit into from
Nov 22, 2020

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Nov 20, 2020

Problems

CmdLine's upgrade command is pretty inconsistent with install and remove:

  • It doesn't describe the changes and present a yes/no prompt to confirm the user wants to proceed
  • It prints double messages "Upgrading modules..." and "About to upgrade..." at the start
  • It prints double messages "Done!" and "Done!" at the end
    Upgrading modules...
    
    About to upgrade...
    
    Installing previously uninstalled mod Kopernicus-BE
    
    Installing "Kopernicus-BE UBEE_1101_44"
    Updating registry
    Committing filesystem changes
    Done!
    Done!
    
  • It says Removing "CKAN.InstalledModule" for each installed module being upgraded

Other problems:

Motivation

As of #3190, ckan upgrade --all now upgrades AD mods to CKAN-managed. While this is probably what users want on balance, it won't be expected at first, so we should tell them that it's about to happen and allow them to cancel.

Cause

Changes

  • Now ckan upgrade prints the changes to be made and asks the user whether to continue
  • Now ckan upgrade no longer prints double messages at start and end
  • Now the "Removing" message prints the module's name instead
  • Now ckan upgrade --all passes the installed modules to be removed to the RelationshipResolver, as per Fix dependency resolution in mod upgrades #3200
  • Now all three commands use a colon instead of an ellipsis for the header before the list of changes
  • Now all three commands throw CancelledActionKraken if the user opts to abort (remove previously just printed and returned)

@HebaruSan HebaruSan added Bug Something is not working as intended Enhancement New features or functionality Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Pull request labels Nov 20, 2020
@DasSkelett
Copy link
Member

I noticed two whitespace inconsistencies:

  • After the "Done!" from the upgrade command there is no new line, so it merges with the shell prompt. I think we should change that User.RaiseMessage() to User.RaiseProgress() like in the other commands, with a \r\n.
  • After the change list from remove there are two new lines, everywhere else there's only one. We prepend \r\n to the "Continue?" question there.
user@pc:/GIT/CKAN/_build$ mono ckan.exe upgrade Astrogator
About to upgrade:

* Re-install: Astrogator v0.10.2

Continue? [Y/n] 

Removing "Astrogator v0.10.2"
Installing "Astrogator v0.10.2"
Updating registry
Committing filesystem changes
Done!user@pc:/GIT/CKAN/_build$ mono ckan.exe remove Astrogator
About to remove:

* Astrogator v0.10.2


Continue? [Y/n] 
Removing Astrogator...
Done!

user@pc:/GIT/CKAN/_build$ mono ckan.exe install Astrogator
About to install:

* Astrogator v0.10.2 (cached)
* LoadingTipsPlus V1.9 (cached)

Continue? [Y/n]

Installing mod "Astrogator v0.10.2"
Installing mod "LoadingTipsPlus V1.9"
Updating registry
Committing filesystem changes
Rescanning GameData
Done!

user@pc:/GIT/CKAN/_build$

We also do funny things for replace, if you feel like fixing this as well in this round:

grafik grafik

user@pc:/GIT/CKAN/_build$ mono ckan.exe replace --all

Replacing modules...

Replacement Science-Full-Reward v5.2 found for NMSG 4.1


Continue? [Y/n]
Downloading "https://github.com/tekaohksp/Science-Full-Reward/releases/download/v5.2/Science-Full-Reward.zip"
0 B/sec - downloading - 0 B left - 100%
Removing "NMSG 4.1"
Installing "Science-Full-Reward v5.2"
Updating registry
Committing filesystem changes
Done!

user@pc:/GIT/CKAN/_build$

@HebaruSan
Copy link
Member Author

Try now?

@DasSkelett
Copy link
Member

Looks good! Thanks!!

@HebaruSan HebaruSan merged commit 58f3ca7 into KSP-CKAN:master Nov 22, 2020
@HebaruSan HebaruSan deleted the fix/cmdline-confirm-upgrade branch November 22, 2020 01:19
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 Enhancement New features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants