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

[Modules] Changed to updated API and also added new feature properties #1490

Closed
wants to merge 8 commits into from
Closed

[Modules] Changed to updated API and also added new feature properties #1490

wants to merge 8 commits into from

Conversation

prasanjeets
Copy link
Contributor

@prasanjeets prasanjeets commented Jun 6, 2022 •
edited
Loading

Description

Thank you for your contribution !

The latest API version for Microsoft.RecoveryServices/vaults and its child resources such as Microsoft.RecoveryServices/vaults/backupconfig and Microsoft.RecoveryServices/vaults/backupPolicies are outdated. at the moment, the latest API version is 2022-02-01. Same has been updated, also updated new feature properties and tested the same.
Please also include the context.
List any dependencies that are required for this change.

Pipeline references

For module/pipeline changes, please create and attach the status badge of your successful run.

Pipeline

Type of Change

Please delete options that are not relevant.

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • My corresponding pipelines / checks run clean and green without any errors or warnings
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (readme)
  • I did format my code

@ghost
Copy link

ghost commented Jun 6, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

eriqua
eriqua requested changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@eriqua eriqua left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Please regenerate all related readme files after changing the templates leveraging the utility in utilities\tools\Set-ModuleReadMe.ps1

@AlexanderSehr AlexanderSehr added enhancement New feature or request [cat] modules category: modules labels Jun 6, 2022
@eriqua eriqua changed the title Changed to updated API and also added new feature properties [Modules] Changed to updated API and also added new feature properties Jun 6, 2022
@prasanjeets prasanjeets requested a review from eriqua June 15, 2022 06:49
@eriqua eriqua mentioned this pull request Jun 15, 2022
Closed
15 tasks
eriqua
eriqua reviewed Jun 16, 2022
View reviewed changes
arm/Microsoft.RecoveryServices/vaults/protectionContainers/readme.md
Comment on lines +40 to +43
### Parameter Usage: `<ParameterPlaceholder>`

// TODO: Fill in Parameter usage

Copy link
Contributor

Choose a reason for hiding this comment

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

Please either add parameter usage if needed or remove this placeholder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you kindly double check? I still see the old code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I can see the correct code in my branch

Copy link
Contributor

Choose a reason for hiding this comment

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

You can. Just update you main to latest & merge it into you branch:

git checkout main
git pull
git checkout 'users/prsaha/apifixrsv#1388'
git merge main

eriqua
eriqua reviewed Jun 16, 2022
View reviewed changes
arm/Microsoft.RecoveryServices/vaults/protectionContainers/readme.md
@@ -1,18 +1,19 @@
# RecoveryServicesProtectionContainer `[Microsoft.RecoveryServices/vaults/protectionContainers]`
# RecoveryServices Vaults ProtectionContainers `[Microsoft.RecoveryServices/vaults/protectionContainers]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# RecoveryServices Vaults ProtectionContainers `[Microsoft.RecoveryServices/vaults/protectionContainers]`
# Recovery Services Vaults Protection Containers `[Microsoft.RecoveryServices/vaults/protectionContainers]`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you kindly double check? I still see the old code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I can see the correct code in my branch

Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment

eriqua
eriqua reviewed Jun 16, 2022
View reviewed changes
arm/Microsoft.RecoveryServices/vaults/protectionContainers/readme.md
Comment on lines +3 to +4
This module deploys RecoveryServices Vaults ProtectionContainers.
// TODO: Replace Resource and fill in description
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This module deploys RecoveryServices Vaults ProtectionContainers.
// TODO: Replace Resource and fill in description
This module deploys a recovery services vault protection container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@prasanjeets prasanjeets requested a review from eriqua June 16, 2022 10:47
eriqua
eriqua reviewed Jun 16, 2022
View reviewed changes
arm/Microsoft.RecoveryServices/vaults/deploy.bicep Outdated
@@ -239,6 +239,7 @@ module rsv_backupConfig 'backupConfig/deploy.bicep' = if (!empty(backupConfig))
storageTypeState: contains(backupConfig, 'storageTypeState') ? backupConfig.storageTypeState : 'Locked'
isSoftDeleteFeatureStateEditable: contains(backupConfig, 'isSoftDeleteFeatureStateEditable') ? backupConfig.isSoftDeleteFeatureStateEditable : true
enableDefaultTelemetry: enableChildTelemetry
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this line as it has been correctly replaced below

Suggested change
enableDefaultTelemetry: enableChildTelemetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

eriqua
eriqua reviewed Jun 16, 2022
View reviewed changes
arm/Microsoft.RecoveryServices/vaults/deploy.bicep Outdated
@@ -195,7 +195,9 @@ module rsv_backupStorageConfiguration 'backupStorageConfig/deploy.bicep' = if (!
recoveryVaultName: rsv.name
storageModelType: backupStorageConfig.storageModelType
crossRegionRestoreFlag: backupStorageConfig.crossRegionRestoreFlag
enableDefaultTelemetry: enableReferencedModulesTelemetry
enableDefaultTelemetry: enableChildTelemetry
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace any occurrence of enableChildTelemetry with enableReferencedModulesTelemetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

eriqua
eriqua requested changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@eriqua eriqua left a comment

Choose a reason for hiding this comment

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

Something is off, the PR now shows 618 file updates. Probably due to a pull from main. Please double check.

@prasanjeets prasanjeets mentioned this pull request Jun 22, 2022
Merged
10 tasks
AlexanderSehr
AlexanderSehr reviewed Jun 22, 2022
View reviewed changes
arm/Microsoft.RecoveryServices/vaults/protectionContainers/protectedItems/readme.md
@@ -1,46 +0,0 @@
# Recovery Service Vault Protection Container Protected Item `[Microsoft.RecoveryServices/vaults/protectionContainers/protectedItems]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the subsequent PR #1572 - why are these files deleted?

@prasanjeets prasanjeets deleted the users/prsaha/apifixrsv#1388 branch July 11, 2022 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderSehr AlexanderSehr AlexanderSehr left review comments

@eriqua eriqua eriqua requested changes

Assignees

@prasanjeets prasanjeets

Labels
[cat] modules category: modules enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prasanjeets @AlexanderSehr @eriqua