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

Add User Setting controlling Path Tokenization #3209

Merged
merged 3 commits into from May 19, 2023

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented May 3, 2023

Some users may not want their paths to be replaced with environment variables when displayed. This PR adds a setting to control whether the paths are tokenized or not.


Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner May 3, 2023 04:09
@github-actions

This comment has been minimized.

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Trenly
Copy link
Contributor Author

Trenly commented May 17, 2023

@yao-msft - When re-running the tests locally I get similar messages for several tests. Is this the same error the pipeline is showing?

D:\Git\winget-cli\src\AppInstallerCLITests\AdminSettings.cpp(19): FAILED:
  REQUIRE( EnableAdminSetting(AdminSetting::LocalManifestFiles) )
due to unexpected exception with message:
  D:\Git\winget-cli\src\AppInstallerCommonCore\Settings.cpp(309)\
  AppInstallerCLITests.exe!00007FF708BC02DE: (caller: 00007FF708E303CB)
  Exception(1) tid(5f04) 800F024B The hash for the file is not present in the
  specified catalog file. The file is likely corrupt or the victim of
  tampering.
      [AppInstaller::Settings::`anonymous-namespace'::SecureSettingsContainer::
  Get(!verData.Found)]

@yao-msft
Copy link
Contributor

The failure test case is:

SettingsAnonymizePathForDisplay/Invalid Value

with:

FAILED:
REQUIRE( userSettingTest.GetWarnings().size() == 1 )
with expansion:
2 == 1
at D:\a\1\s\src\AppInstallerCLITests\UserSettings.cpp(243)

Maybe set a breakpoint to see why 2 warnings instead of 1 warnings?

@Trenly
Copy link
Contributor Author

Trenly commented May 18, 2023

The failure test case is:

SettingsAnonymizePathForDisplay/Invalid Value

with:

FAILED: REQUIRE( userSettingTest.GetWarnings().size() == 1 ) with expansion: 2 == 1 at D:\a\1\s\src\AppInstallerCLITests\UserSettings.cpp(243)

Maybe set a breakpoint to see why 2 warnings instead of 1 warnings?

It seems to be complaining about data type when I try to load the settings file. I'm taking a guess that its because the string value isn't quoted and is malformed JSON. If the latest commit doesn't fix the test, I'm not sure what the second error would be

Edit: Validated and tested locally by logging the exact errors thrown

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@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 5d6c805 into microsoft:master May 19, 2023
8 checks passed
@Trenly Trenly deleted the AnonymizeSetting branch May 19, 2023 02:22
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.

None yet

2 participants