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

azurerm_site_recovery_replication_recovery_plan: keep the order of recovery_group #22348

Merged
merged 11 commits into from Jul 20, 2023
Merged

azurerm_site_recovery_replication_recovery_plan: keep the order of recovery_group #22348

merged 11 commits into from Jul 20, 2023

Conversation

ziyeqf
Copy link
Contributor

@ziyeqf ziyeqf commented Jul 3, 2023 •
edited

fix #22323
Screenshot 2023-07-03 at 17 31 47

@ziyeqf
azurerm_site_recovery_replication_recovery_plan: keep the order of …
3411bab 
…`recovery_group`

Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf ziyeqf force-pushed the tengzh/issue/recoveryplan_group_order branch from b630242 to 3411bab Compare July 3, 2023 05:12
@ziyeqf ziyeqf mentioned this pull request Jul 3, 2023
Closed
1 task
@ziyeqf
update testcase
59e4679 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf ziyeqf marked this pull request as ready for review July 3, 2023 09:32
tombuildsstuff
tombuildsstuff reviewed Jul 3, 2023
View reviewed changes
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated
@@ -258,6 +261,8 @@ A `recovery_group` block supports the following:

* `type` - (Required) The Recovery Plan Group Type. Possible values are `Boot`, `Failover` and `Shutdown`.

**Note:** The List of `recovery_group` must be in the order of `Shutdown`-`Failover`-`Boot`.
Copy link
Member

Choose a reason for hiding this comment

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

should these be modelled differently to account for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have updated per comments, please take another look:)

@ziyeqf
update per comments
50bd332 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf
Copy link
Contributor Author

ziyeqf commented Jul 4, 2023

triggered a test on TC and will upload the result here

@github-actions github-actions bot added size/XL and removed size/L labels Jul 4, 2023
@ziyeqf
update code
a5269ff 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf
golint
14772ce 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf ziyeqf force-pushed the tengzh/issue/recoveryplan_group_order branch from e95a168 to d858643 Compare July 4, 2023 07:20
@ziyeqf
update testcase
aa0b8f1 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf ziyeqf force-pushed the tengzh/issue/recoveryplan_group_order branch from 36ccc7a to aa0b8f1 Compare July 4, 2023 11:15
@ziyeqf
Copy link
Contributor Author

ziyeqf commented Jul 5, 2023

Screenshot 2023-07-05 at 09 53 07

the schema lint is failing because the new schema has the following

Required: features.FourPointOhBeta(),
Optional: !features.FourPointOhBeta(),
Computed: !features.FourPointOhBeta(),

Can we ignore the failure?

horvatal
horvatal reviewed Jul 5, 2023
View reviewed changes
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated

-> **NOTE:** `shutdown_recovery_group` will be required in the next major version of the AzureRM Provider.

* `failover_recovery_group` - (Optional) One or more `failover_recovery_group` block defined as below.
Copy link

@horvatal horvatal Jul 5, 2023 •
edited

Choose a reason for hiding this comment

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

This is not consistent with the Microsoft.RecoveryServices/vaults/replicationRecoveryPlans@2022-10-01 API. If I create multiple failover or shutdown groups, I receive an error: "recommendedAction": "Ensure there is only a single shutdown and a single failover group and that virtual machines are present in only one boot group." Only multiple boot groups are allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, code updated

@ziyeqf
limit shutdown_recovery_group and failover_recovery_group to 1
d890c13 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf
update code
40b5a24 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf ziyeqf mentioned this pull request Jul 7, 2023
Closed
1 task
@ziyeqf ziyeqf force-pushed the tengzh/issue/recoveryplan_group_order branch from 431e293 to 40b5a24 Compare July 7, 2023 02:17
@ziyeqf
golint
caebbf8 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
manicminer
manicminer requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@manicminer manicminer left a comment •
edited

Choose a reason for hiding this comment

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

@ziyeqf Can you look at the linting error? Unfortunately we can't merge a fixable failure.

@ziyeqf
lint
f82f6df 
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
manicminer
manicminer approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@manicminer manicminer left a comment •
edited

Choose a reason for hiding this comment

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

Thanks @ziyeqf, just some minor wording tweaks but this otherwise LGTM! 🪐

Screenshot 2023-07-20 at 23 00 55

internal/services/recoveryservices/site_recovery_replication_recovery_plan_resource.go Outdated Show resolved Hide resolved
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated Show resolved Hide resolved
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated Show resolved Hide resolved
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated Show resolved Hide resolved
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated Show resolved Hide resolved
website/docs/r/site_recovery_replication_recovery_plan.html.markdown Outdated Show resolved Hide resolved
@manicminer manicminer added this to the v3.66.0 milestone Jul 20, 2023
@manicminer manicminer merged commit 44d26c6 into hashicorp:main Jul 20, 2023
22 checks passed
manicminer added a commit that referenced this pull request Jul 20, 2023
@manicminer
Changelog for #22348
ff30d33
@ziyeqf ziyeqf deleted the tengzh/issue/recoveryplan_group_order branch December 20, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff left review comments

@horvatal horvatal horvatal left review comments

@manicminer manicminer manicminer approved these changes

Assignees
No one assigned
Labels
documentation service/recovery-services size/XL
Projects
None yet
Milestone
v3.66.0
Development

Successfully merging this pull request may close these issues.

azurerm_site_recovery_replication_recovery_plan does not keep order of boot groups
5 participants
@ziyeqf @manicminer @tombuildsstuff @horvatal @jackofallops