Skip to content

Remove RequireLicenseAcceptance default value #11512

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
Merged
TravisEz13 merged 3 commits into from
Jan 7, 2020
Merged

Remove RequireLicenseAcceptance default value #11512

TravisEz13 merged 3 commits into from
Jan 7, 2020

Conversation

@ThomasNieto
Copy link
Contributor

@ThomasNieto ThomasNieto commented Jan 7, 2020 •
edited
Loading

PR Summary

This PR is to fix #11491 by removing the default value for RequireLicenseAcceptance switch parameter on New-ModuleManifest.

PR Context

When RequireLicenseAcceptance switch parameter was added, it had a default value of true. This caused new module manifests to have a unintended result. It also required users to use an awkward syntax to disable it (-RequireLicenseAcceptance:$false).

PR Checklist

@ghost ghost assigned TravisEz13 Jan 7, 2020
iSazonov
iSazonov reviewed Jan 7, 2020
View reviewed changes
iSazonov
iSazonov reviewed Jan 7, 2020
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

Please open new issue in PowerShell-Docs repo.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 7, 2020
@iSazonov iSazonov added this to the GA-consider milestone Jan 7, 2020
@ThomasNieto ThomasNieto mentioned this pull request Jan 7, 2020
Closed
10 tasks
@ThomasNieto
Copy link
Contributor Author

ThomasNieto commented Jan 7, 2020

@iSazonov I've updated the PR to reference docs issue: MicrosoftDocs/PowerShell-Docs#5275

@TravisEz13 TravisEz13 added the Breaking-Change breaking change that may affect users label Jan 7, 2020
@TravisEz13
Copy link
Member

TravisEz13 commented Jan 7, 2020

Marked this a breaking change because it changes the default, which may cause unexpected results for users familiar with the existing behavior.

@TravisEz13 TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jan 7, 2020
@TravisEz13
Copy link
Member

TravisEz13 commented Jan 7, 2020

Since this breaking change involves licensing, I'm asking the committee to review it before we take the change.

@TravisEz13 TravisEz13 removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jan 7, 2020
@TravisEz13
Copy link
Member

TravisEz13 commented Jan 7, 2020

This change was introduced in 7, so, it is not a breaking change from 6.2 and doesn't require committee review.

@TravisEz13 TravisEz13 merged commit b25e67e into PowerShell:master Jan 7, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 7, 2020

Should we remove Breaking label too?

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 7, 2020

@ThomasNieto Thank for your contribution!

@TravisEz13
Copy link
Member

TravisEz13 commented Jan 7, 2020

@iSazonov This is breaking from previous 7 previews. We have labels those changes in the past.

@TravisEz13 TravisEz13 added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Jan 7, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 7, 2020

@TravisEz13 Will we have full change list (from 6.x to 7.0) in CHANGE.log? In the case such labels will be not correct.

@TravisEz13
Copy link
Member

TravisEz13 commented Jan 7, 2020

How should we denote an inter-release breaking change?

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 7, 2020

This is breaking from previous 7 previews.

My concern is about this. I think it is not a breaking change if we change something in 7.0 Preview.2 that came from 7.0 Preview.1. I believe breaking change baseline for 7.0 GA is latest 6.x version.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 7, 2020

Agree with @iSazonov's assessment. Breaking change is from stable release perspective.

@TravisEz13 TravisEz13 removed Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log labels Jan 7, 2020
@TravisEz13
Copy link
Member

TravisEz13 commented Jan 7, 2020

Created a PR to clarify the guidance, as this is not the definition of a breaking change we have used previously: #11516

daxian-dbw pushed a commit that referenced this pull request Jan 10, 2020
@ThomasNieto @daxian-dbw
Remove RequireLicenseAcceptance default value (#11512)
4279500
@daxian-dbw daxian-dbw modified the milestones: GA-approved, 7.0.0-rc.2 Jan 11, 2020
@ghost
Copy link

ghost commented Jan 16, 2020

🎉v7.0.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jan 16, 2020
Closed
@TravisEz13 TravisEz13 mentioned this pull request Oct 12, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@iSazonov iSazonov iSazonov approved these changes

Assignees

@TravisEz13 TravisEz13

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.0.0-rc.2

Development

Successfully merging this pull request may close these issues.

RequireLicenseAcceptance set to $true automatically on PS 7

6 participants

@ThomasNieto @TravisEz13 @iSazonov @SteveL-MSFT @daxian-dbw @adityapatwardhan