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.

Sign up for GitHub

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

Jump to bottom

Add Experimental Argument for UninstallPrevious in Upgrade Flow #2755

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Add Experimental Argument for UninstallPrevious in Upgrade Flow #2755

merged 3 commits into from
Jan 10, 2023

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Dec 7, 2022 •
edited by ghost
Loading

Left this just for the upgrade flow (and Installs that switched to using the upgrade flow). May potentially want to consider enabling for default install flow (for when --force) is used. Depends on user feedback and use cases

This will cause a merge conflict with #2733 due to the Experimental Features Enum; If pinning is merged first, I will be happy to rebase this

Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner December 7, 2022 19:23
@yao-msft
Copy link
Contributor

yao-msft commented Dec 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Trenly
Copy link
Contributor Author

Trenly commented Dec 7, 2022

I need to start checking more thoroughly for typos before I push . . .

@florelis
Copy link
Member

florelis commented Dec 8, 2022

This will cause a merge conflict with #2733 due to the Experimental Features Enum; If pinning is merged first, I will be happy to rebase this

Looks like I beat you to it, sorry!

@Trenly
Copy link
Contributor Author

Trenly commented Dec 8, 2022

This will cause a merge conflict with #2733 due to the Experimental Features Enum; If pinning is merged first, I will be happy to rebase this

Looks like I beat you to it, sorry!

No apology needed :)
I was hoping pinning would be first anyways

yao-msft
yao-msft reviewed Jan 9, 2023
View reviewed changes
Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry we reviewed this late as we just had our v1.4 release candidate created last week.

The change looks good. Please merge to the latest when time available. Thanks.

And an open question, since import command also converts to upgrade when needed, does power user want to remove previous for them as well? I guess not, but I'll just leave the question here.

src/AppInstallerCLICore/Workflows/InstallFlow.cpp Show resolved Hide resolved
@Trenly
Copy link
Contributor Author

Trenly commented Jan 10, 2023

And an open question, since import command also converts to upgrade when needed, does power user want to remove previous for them as well? I guess not, but I'll just leave the question here.

Possibly; I think it may be best to wait and see if we get feedback on it since I'm not sure how useful it will be in the import scenario

@Trenly Trenly requested a review from yao-msft January 10, 2023 03:10
@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft yao-msft merged commit 5c6ba96 into microsoft:master Jan 10, 2023
@Trenly Trenly deleted the UninstallPreviousArg branch January 10, 2023 22:08
@Trenly Trenly mentioned this pull request Jan 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yao-msft yao-msft yao-msft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Trenly @yao-msft @florelis