-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adding the client code for supporting experiments #5034
base: master
Are you sure you want to change the base?
Changes from 2 commits
20d1d66
3e66324
e9d77e9
f95c26c
1d7fcd0
1720e20
2072721
9a41fff
ff0ac7b
9dbc7d4
bdf8c90
dee65a9
f26ae41
0de0174
e1bafb4
7e12f7f
c76d4d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
|
||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT License. | ||
#include "pch.h" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,35 +13,30 @@ namespace AppInstaller::Settings | |
{ | ||
ExperimentState GetExperimentStateInternal(Experiment::Key key, const UserSettings& userSettings) | ||
{ | ||
if (key == Experiment::Key::None) | ||
{ | ||
return { true, ExperimentToggleSource::Default }; | ||
} | ||
|
||
if (!GroupPolicies().IsEnabled(TogglePolicy::Policy::Experiments)) | ||
{ | ||
AICLI_LOG(Core, Info, << | ||
"Experiment " << Experiment::GetExperiment(key).Name() << | ||
AICLI_LOG(Core, Info, << "Experiment " << Experiment::GetExperiment(key).Name() << | ||
" is disabled due to group policy: " << TogglePolicy::GetPolicy(TogglePolicy::Policy::Experiments).RegValueName()); | ||
return { false, ExperimentToggleSource::Policy }; | ||
AmelBawa-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
auto experiments = userSettings.Get<Setting::Experiments>(); | ||
if (key == Experiment::Key::None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this back to first. While I'm not sure that we need a |
||
{ | ||
return { false, ExperimentToggleSource::Default }; | ||
} | ||
|
||
auto experiment = Experiment::GetExperiment(key); | ||
AmelBawa-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auto userSettingsExperiments = userSettings.Get<Setting::Experiments>(); | ||
auto jsonName = std::string(experiment.JsonName()); | ||
if (experiments.find(jsonName) != experiments.end()) | ||
if (userSettingsExperiments.find(jsonName) != userSettingsExperiments.end()) | ||
{ | ||
auto isEnabled = experiments[jsonName]; | ||
AICLI_LOG(Core, Info, << | ||
"Experiment " << Experiment::GetExperiment(key).Name() << | ||
" is set to " << isEnabled << " in user settings"); | ||
auto isEnabled = userSettingsExperiments[jsonName]; | ||
AICLI_LOG(Core, Info, << "Experiment " << experiment.Name() << " is set to " << (isEnabled ? "true" : "false") << " in user settings"); | ||
return { isEnabled, ExperimentToggleSource::UserSetting }; | ||
} | ||
|
||
auto isEnabled = AppInstaller::Experiment::IsEnabled(experiment.GetKey()); | ||
AICLI_LOG(Core, Info, << | ||
"Experiment " << Experiment::GetExperiment(key).Name() << | ||
" is set to " << isEnabled); | ||
AICLI_LOG(Core, Info, << "Experiment " << experiment.Name() << " is set to " << (isEnabled ? "true" : "false")); | ||
return { isEnabled, ExperimentToggleSource::Default }; | ||
} | ||
|
||
|
@@ -85,7 +80,7 @@ namespace AppInstaller::Settings | |
switch (key) | ||
{ | ||
case Key::CDN: | ||
return Experiment{ "CDN experiment", "CDN", "https://aka.ms/winget-settings", "CDN"}; | ||
return Experiment{ "winget source CDN experiment", "CDN", "https://aka.ms/winget-settings", "CDN"}; | ||
#ifndef AICLI_DISABLE_TEST_HOOKS | ||
case Key::TestExperiment: | ||
return Experiment{ "Test experiment", "TestExperiment", "https://aka.ms/winget-settings", "TestExperiment" }; | ||
|
@@ -99,7 +94,7 @@ namespace AppInstaller::Settings | |
{ | ||
std::vector<Experiment> result; | ||
|
||
for (Key_t i = 0x1; i < static_cast<Key_t>(Key::Max); i = i << 1) | ||
for (Key_t i = 0x1; i < static_cast<Key_t>(Key::Max); ++i) | ||
{ | ||
result.emplace_back(GetExperiment(static_cast<Key>(i))); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,8 +10,8 @@ namespace AppInstaller::Settings | |||||
enum ExperimentToggleSource | ||||||
{ | ||||||
Default = 0, | ||||||
Policy = 1, | ||||||
UserSetting = 2, | ||||||
Policy, | ||||||
UserSetting, | ||||||
}; | ||||||
|
||||||
struct ExperimentState | ||||||
|
@@ -30,8 +30,8 @@ namespace AppInstaller::Settings | |||||
{ | ||||||
enum class Key : unsigned | ||||||
{ | ||||||
None = 0x0, | ||||||
CDN = 0x1, | ||||||
None = 0, | ||||||
CDN, | ||||||
Max, | ||||||
|
||||||
#ifndef AICLI_DISABLE_TEST_HOOKS | ||||||
|
@@ -41,23 +41,23 @@ namespace AppInstaller::Settings | |||||
|
||||||
using Key_t = std::underlying_type_t<Key>; | ||||||
|
||||||
Experiment(std::string_view name, std::string_view jsonName, std::string_view link, std::string key) : | ||||||
m_name(name), m_jsonName(jsonName), m_link(link), m_key(key) {} | ||||||
Experiment(std::string name, std::string jsonName, std::string link, std::string key) : | ||||||
m_name(std::move(name)), m_jsonName(jsonName), m_link(std::move(link)), m_key(std::move((key))) {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Accompanied by making |
||||||
|
||||||
static ExperimentState GetState(Key feature); | ||||||
static ExperimentState GetStateInternal(Key feature); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What makes it internal if it is public? |
||||||
static Experiment GetExperiment(Key key); | ||||||
static std::vector<Experiment> GetAllExperiments(); | ||||||
|
||||||
std::string_view Name() const { return m_name; } | ||||||
std::string Name() const { return m_name; } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
And the rest as |
||||||
Utility::LocIndView JsonName() const { return m_jsonName; } | ||||||
std::string_view Link() const { return m_link; } | ||||||
std::string Link() const { return m_link; } | ||||||
std::string GetKey() const { return m_key; } | ||||||
|
||||||
private: | ||||||
std::string_view m_name; | ||||||
std::string m_name; | ||||||
Utility::LocIndView m_jsonName; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not need to be a localized value, does it? |
||||||
std::string_view m_link; | ||||||
std::string m_link; | ||||||
std::string m_key; | ||||||
}; | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of extra tests that would be good: