★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/8318Nouvelle recherche | Portail wanayoo
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 `Enable-ExperimentalFeature` and `Disable-ExperimentalFeature` cmdlets #8318

Merged

Conversation

@SteveL-MSFT
Copy link
Member

commented Nov 20, 2018

PR Summary

Add Enable-ExperimentalFeature and Disable-ExperimentalFeature cmdlets. Remove -ListAvailable from Get-ExperimentalFeature (Breaking Change). Add ArgumentCompleter for Get-ExperimentalFeature cmdlet. Refactor some existing Experimental Feature tests. Make ConfigScope public and renamed SystemWide to AllUsers.

Implements PowerShell/PowerShell-RFC#148

PR Checklist

@rjmholt
Copy link
Member

left a comment

Have left some small comments, but it all looks pretty good to me. We should also make sure we start the documentation process before merging this PR.

Show resolved Hide resolved ...on/engine/ExperimentalFeature/EnableDisableExperimentalFeatureCommand.cs Outdated
Show resolved Hide resolved ...on/engine/ExperimentalFeature/EnableDisableExperimentalFeatureCommand.cs
Show resolved Hide resolved ...on/engine/ExperimentalFeature/EnableDisableExperimentalFeatureCommand.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/PSConfiguration.cs
Show resolved Hide resolved src/System.Management.Automation/engine/PSConfiguration.cs
Show resolved Hide resolved src/System.Management.Automation/engine/PSConfiguration.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/PSConfiguration.cs
Show resolved Hide resolved src/System.Management.Automation/engine/PSConfiguration.cs Outdated
Show resolved Hide resolved ...l/engine/ExperimentalFeature/EnableDisable-ExperimentalFeature.Tests.ps1
Show resolved Hide resolved ...on/engine/ExperimentalFeature/EnableDisableExperimentalFeatureCommand.cs
src/System.Management.Automation/engine/PSConfiguration.cs Outdated
features.Remove(featureName);
}

WriteValueToFile<string[]>(scope, "ExperimentalFeatures", features.ToArray());

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Nov 20, 2018

Contributor

We should write to file only if the features array changes.

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Nov 20, 2018

Author Member

Seems like this should be a separate issue for WriteValueToFile(). Opened #8325

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Nov 20, 2018

Contributor

Actually, I disagree. We can save even running WriteValueToFile() code, which involves opening the file and reading data, by simply keeping track of whether we actually have a change to record.

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Nov 20, 2018

Contributor

Unless we are concerned with file contention. If so, then I assume the last write to should win, and it makes sense to ensure the file actually reflects the current config state.

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:experimental-feature-cmdlets branch to c8743f3 Nov 20, 2018

@PaulHigin
Copy link
Contributor

left a comment

LGTM

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 21, 2018

Can we assign two CL- labels?

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

@PowerShell/powershell-committee reviewed this as part of the RFC review and approves of the breaking change and current cmdlet design

SteveL-MSFT added some commits Nov 5, 2018

[feature]
add enable/disable-experimentalfeature cmdlets
updated existing tests
[feature]
address CodeFactor issues
[feature]
fix AppVeyor failure

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:experimental-feature-cmdlets branch from c8743f3 to 4ba6cbc Nov 29, 2018

[feature]
address Paul and Rob's feedback

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:experimental-feature-cmdlets branch from 4ba6cbc to 6727dd7 Nov 29, 2018

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

Doc PR submitted and linked

@iSazonov
Copy link
Collaborator

left a comment

We lost the comment perUserConfigFile -> _perUserConfigFile for private.

iSazonov and others added some commits Nov 30, 2018

Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>

iSazonov and others added some commits Nov 30, 2018

Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
@iSazonov
Copy link
Collaborator

left a comment

@SteveL-MSFT You skipped two my comments for Should -BeFalse

iSazonov and others added some commits Nov 30, 2018

Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
Update test/powershell/engine/ExperimentalFeature/Get-ExperimentalFea…
…ture.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@iSazonov it was hidden under GitHub collapsing it. Thanks for calling it out. Fixed.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2018

@SteveL-MSFT HelpUri do not pass tests.

@iSazonov
Copy link
Collaborator

left a comment

It seems we need remove the cmdlets from release versions.

Update src/System.Management.Automation/engine/ExperimentalFeature/En…
…ableDisableExperimentalFeatureCommand.cs

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>

vexx32 and others added some commits Dec 4, 2018

Update src/System.Management.Automation/engine/ExperimentalFeature/En…
…ableDisableExperimentalFeatureCommand.cs

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
[feature]
address Dongbo's feedback
@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

@SteveL-MSFT One test for Enable/Disable-ExperimentalFeature cmdlets failed in CI runs. Can you please take a look?

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

@daxian-dbw I forgot to update the tests to reflect that the cmdlet no longer outputs the feature object. Will update.

[feature]
fix tests to reflect that no object is output from cmdlets on success

@daxian-dbw daxian-dbw merged commit 52aef57 into PowerShell:master Dec 5, 2018

6 checks passed

CodeFactor 71 issues fixed.
Details
PowerShell-CI-linux #PR-8318-20181205.01 succeeded
Details
PowerShell-CI-macos #PR-8318-20181205.01 succeeded
Details
PowerShell-CI-spelling #PR-8318-20181205.01 succeeded
Details
PowerShell-CI-windows #PR-8318-20181205.01 succeeded
Details
license/cla All CLA requirements met.
Details

@daxian-dbw daxian-dbw referenced this pull request Dec 14, 2018

Merged

Add about_PowerShell_Config #3238

2 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.