Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Implement uninstall tool command. #8615

Merged
merged 1 commit into from
Feb 20, 2018
Merged

Implement uninstall tool command. #8615

merged 1 commit into from
Feb 20, 2018

Conversation

peterhuene
Copy link

This commit implements the uninstall tool command.

The uninstall tool command is responsible for uninstalling global tools that
are installed with the install tool command.

This commit heavily refactors the ToolPackage and ShellShim namespaces to
better support the operations required for the uninstall command.

Several string resources have been updated to be more informative or to correct
oddly structured sentences.

This commit also fixes --version on the install command not supporting ranges
and wildcards.

Fixes #8549.

Issue #8485 is partially fixed by this commit (--prerelease is not yet
implemented).

@peterhuene peterhuene requested review from wli3 and a team February 14, 2018 23:11
@peterhuene
Copy link
Author

peterhuene commented Feb 15, 2018

So two tests appear to be failing on the localized build because (I assume) it uses the existing translation for these resource strings; given that the arguments to the formatted strings have decreased, using the existing translation would result in this failure (i.e. we are now passing in a single argument when an existing translated string would expect {1} to work).

When modifying a string with an existing translation such that the number of formatting arguments decreases, is it best to just rename the string resource, thereby ensuring the localized build tests with the correct number of expected arguments?

@nguerrera
Copy link
Contributor

Yes, rename if fewer args are passed.

{
if (Reporter.IsVerbose)

This comment was marked as spam.

This comment was marked as spam.

@peterhuene
Copy link
Author

Looking into the RHEL failure.

@livarcocc livarcocc added this to the 2.1.3xx milestone Feb 15, 2018
@wli3
Copy link

wli3 commented Feb 16, 2018

The following do not need to address in this PR

I just realize a lot of stuff we don't have to write mock, The current mock has a lot of duplicated code with production, which is a sign that they can be extracted out.

Since the only place we cannot easily test is the part with nuget(the original obtainer part) and shim creation, everything else can be test against these mocks (may be obtainer mock need to write the xml file, since the xml file is considered a contract, not implementation detail anymore). In a sense our adapter layer is too high, a lot of them can be considered core logic.

The logic of transaction, folder location, rename folder, delete assets, can all be test against package obatiner mock, shim creation mock, and file system mock. The above logic can be tests all in memory. In this way, we don't have to write mock for package store, duplicate transaction logic in mock. To test InstallCommand, we can test again concrete transaction logic but just cut off the layer that deal with file IO.

Maybe we can code list logic as such.

@peterhuene
Copy link
Author

The RHEL failure is legitimate and appears to be a race. I'll try to investigate and fix tomorrow.

This commit implements the `uninstall tool` command.

The `uninstall tool` command is responsible for uninstalling global tools that
are installed with the `install tool` command.

This commit heavily refactors the ToolPackage and ShellShim namespaces to
better support the operations required for the uninstall command.

Several string resources have been updated to be more informative or to correct
oddly structured sentences.

This commit also fixes `--version` on the install command not supporting ranges
and wildcards.

Fixes #8549.

Issue #8485 is partially fixed by this commit (`--prerelease` is not yet
implemented).
@peterhuene
Copy link
Author

peterhuene commented Feb 20, 2018

I've pushed up an amendment that prevents the transaction manager from creating a timer to timeout the transactions. This address the inconsistent RHEL failure and also the issue of very long tool package downloads as they would have been aborted as a result.

@peterhuene peterhuene merged commit 7177792 into dotnet:master Feb 20, 2018
@peterhuene peterhuene deleted the uninstall-command branch February 21, 2018 21:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uninstall, in dotnet install tool -g
4 participants