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.

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

Edit Get & Set ShortTermRetentionPolicy For Adding New Parameter: DiffBackupIntervalInHours #12562

Merged
merged 9 commits into from
Oct 15, 2020
Merged

Edit Get & Set ShortTermRetentionPolicy For Adding New Parameter: DiffBackupIntervalInHours #12562

merged 9 commits into from
Oct 15, 2020

Conversation

lululilliancoding
Copy link
Contributor

@lululilliancoding lululilliancoding commented Aug 4, 2020 •
edited
Loading

Description

ShortTermRetentionPolicy Get and Set cmdlets to include new parameter DiffBackupIntervalInHours.

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

Draft ShortTermRetentionPolicy Get and Set cmdlets to include new par…
9ed42d2 
…ameter DiffBackupIntervalInHours PR (except record test session).
@adxsdkps
Copy link
Collaborator

adxsdkps commented Aug 4, 2020

Can one of the admins verify this patch?

@lululilliancoding lululilliancoding marked this pull request as ready for review August 4, 2020 05:23
@isra-fel isra-fel self-assigned this Aug 6, 2020
@isra-fel
Copy link
Member

Adding "DO NOT MERGE" since SDK is not ready

isra-fel
isra-fel requested changes Sep 10, 2020
View reviewed changes
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

To be specific, what I suggest are two changes to all the nullable parameters.

src/Sql/Sql/Database Backup/Cmdlet/SetAzureRmSqlDatabaseBackupShortTermRetentionPolicy.cs Outdated Show resolved Hide resolved
src/Sql/Sql/Database Backup/Cmdlet/SetAzureRmSqlDatabaseBackupShortTermRetentionPolicy.cs Outdated Show resolved Hide resolved
Lillian Liu and others added 3 commits October 13, 2020 12:48
Removed .Value to allowe either retentionDays or diffBackupInterval c…
9aefa96 
…an be set individually.

Added its related scenario tests and re-record; Added one related example;
Add Position back to avoid breaking change;
c3c94cf 
Add retentionDays nullable changes exception;
Re-upload records.
undid position of DiffBackupIntervalInHours
da515dc
isra-fel
isra-fel approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

I undid the position of DiffBackupIntervalInHours so that static analysis won't complain about "too many positional parameters".
I've also regenerated help docs so that the diff of src/Sql/Sql/help/Set-AzSqlDatabaseBackupShortTermRetentionPolicy.md makes more sense.

@lululilliancoding
Copy link
Contributor Author

I undid the position of DiffBackupIntervalInHours so that static analysis won't complain about "too many positional parameters".
I've also regenerated help docs so that the diff of src/Sql/Sql/help/Set-AzSqlDatabaseBackupShortTermRetentionPolicy.md makes more sense.

That's making sense. Thanks a lot!

@lululilliancoding
Copy link
Contributor Author

lululilliancoding commented Oct 14, 2020 •
edited
Loading

The pipeline hit "run more than 180 minutes" error. I just clicked re-run.

I see 'S177 (2020-10-27) Major Release'. Is this the time to release this new cmdlets? Can customer use it before 10/27?

@isra-fel
Copy link
Member

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@isra-fel isra-fel merged commit 34467c6 into Azure:master Oct 15, 2020
isra-fel added a commit that referenced this pull request Oct 16, 2020
@isra-fel
Revert "Edit Get & Set ShortTermRetentionPolicy For Adding New Parame…
be44412 
…ter: DiffBackupIntervalInHours (#12562)"

This reverts commit 34467c6.
lululilliancoding added a commit that referenced this pull request Oct 16, 2020
@lululilliancoding
Revert "Edit Get & Set ShortTermRetentionPolicy For Adding New Parame…
5b06156 
…ter: DiffBackupIntervalInHours (#12562)"

This reverts commit 34467c6.
@lululilliancoding lululilliancoding mentioned this pull request Oct 16, 2020
Merged
wyunchi-ms pushed a commit that referenced this pull request Oct 17, 2020
@lululilliancoding
Revert "Edit Get & Set ShortTermRetentionPolicy For Adding New Parame…
47c91e0 
…ter: DiffBackupIntervalInHours (#12562)" (#13250)

This reverts commit 34467c6.
isra-fel
isra-fel reviewed Oct 19, 2020
View reviewed changes
src/Sql/Sql/ChangeLog.md
* Added Force parameter to `New-AzSqlDatabase`
* Updated ActiveDirectoryOnlyAuthentication cmdlets for server and instance to include ResourceId and InputObject
* Added support for Managed HSM Uris for SQL DB and Managed Instance
* Added DiffBackupIntervalInHours to `Set-AzSqlDatabaseBackupShortTermRetentionPolicy`
Copy link
Member

@isra-fel isra-fel Oct 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I just noticed that the change logs of past versions were deleted accidently in this PR. Please be careful when you submit another PR. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isra-fel isra-fel isra-fel approved these changes

Assignees

@isra-fel isra-fel

@lululilliancoding lululilliancoding

Labels
Breaking Change Release This PR contains breaking change Waiting for CI :shipit:
Projects
None yet
Milestone
S177 (2020-10-27) Major Release
Development

Successfully merging this pull request may close these issues.
3 participants
@lululilliancoding @adxsdkps @isra-fel