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

Show Enabled Admin Settings in --info #2901

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Feb 1, 2023


This is only a partial implementation, as the user settings are much more complicated. The table is only output if there are AdminSettings enabled, and only shows the admin settings that are enabled

image

@Trenly Trenly requested a review from a team as a code owner February 1, 2023 13:55
@yao-msft
Copy link
Contributor

yao-msft commented Feb 1, 2023

@denelon, does this align with what you visioned for showing admin settings? Basically this is showing a list of enabled admin settings, those not enabled were not shown.

@yao-msft
Copy link
Contributor

yao-msft commented Feb 1, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@denelon
Copy link
Contributor

denelon commented Feb 1, 2023

I was actually thinking about whether it makes sense to show all of them and their state. Group policy objects are either "Enabled", "Disabled", or "Not Configured". I don't know that it makes sense to display them all or not. It could be inconsistent to not display the ones that aren't configured, but their states is a tuple so maybe it's OK.

Links
---------------------------------------------------------------------------
Privacy Statement   https://aka.ms/winget-privacy
License Agreement   https://aka.ms/winget-license
Third Party Notices https://aka.ms/winget-3rdPartyNotice
Homepage            https://aka.ms/winget
Windows Store Terms https://www.microsoft.com/en-us/storedocs/terms-of-sale

Group Policy                                      State
----------------------------------------------------------
Enable Windows App Installer Local Manifest Files Enabled
Enable Windows App Installer Hash Override        Disabled

Admin Settings                                    State
----------------------------------------------------------
BypassCertificatePinningForMicrosoftStore         Disabled
InstallerHashOverride                             Disabled
LocalArchiveMalwareScanOverride                   Disabled
LocalManifestFiles                                Enabled

@Trenly
Copy link
Contributor Author

Trenly commented Feb 1, 2023

I was actually thinking about whether it makes sense to show all of them and their state. Group policy objects are either "Enabled", "Disabled", or "Not Configured". I don't know that it makes sense to display them all or not. It could be inconsistent to not display the ones that aren't configured, but their states is a tuple so maybe it's OK.

Links
---------------------------------------------------------------------------
Privacy Statement   https://aka.ms/winget-privacy
License Agreement   https://aka.ms/winget-license
Third Party Notices https://aka.ms/winget-3rdPartyNotice
Homepage            https://aka.ms/winget
Windows Store Terms https://www.microsoft.com/en-us/storedocs/terms-of-sale

Group Policy                                      State
----------------------------------------------------------
Enable Windows App Installer Local Manifest Files Enabled
Enable Windows App Installer Hash Override        Disabled

Admin Settings                                    State
----------------------------------------------------------
BypassCertificatePinningForMicrosoftStore         Disabled
InstallerHashOverride                             Disabled
LocalArchiveMalwareScanOverride                   Disabled
LocalManifestFiles                                Enabled

It's a small change to show all of them. Can have it done in ~5 minutes.

My thought was that group policies are only shown if they are present and configured. There are more than 2 group policies available, but since only 2 are configured, they are the only ones shown in the list.

For Admin settings they are always present and configured (default to false); It could become a very cluttered output if they were all shown. I could see, perhaps, a middle ground where they are hidden by default, but if any one of them is enabled then show all of them with state.

@denelon - Just need a decision on what you think is best UX

  1. Show all admin settings with their status all the time
  2. Show all admin settings with their status only when any admin setting is enabled
  3. Show only the admin settings which are currently enabled

@Trenly
Copy link
Contributor Author

Trenly commented Feb 1, 2023

Had a sync with denelon separately, and updated per his comments

@yao-msft
Copy link
Contributor

yao-msft commented Feb 2, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

:shipit:

@yao-msft
Copy link
Contributor

yao-msft commented Feb 3, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -67,15 +67,15 @@ namespace AppInstaller::CLI
auto info = context.Reporter.Info();
info << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I guess this could be removed too with the latest change

@yao-msft yao-msft merged commit c49ef19 into microsoft:master Feb 6, 2023
@Trenly Trenly deleted the ShowAdminSettings branch August 30, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants