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

New Resource: azurerm_data_protection_backup_vault_customer_managed_key #28679

Merged
merged 6 commits into from
Feb 14, 2025

Conversation

catriona-m
Copy link
Member

@catriona-m catriona-m commented Feb 4, 2025

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Adding the new resource azurerm_data_protection_backup_vault_customer_managed_key

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)
image

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #25413

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@catriona-m catriona-m marked this pull request as ready for review February 5, 2025 09:41
@catriona-m catriona-m marked this pull request as draft February 5, 2025 10:59
@catriona-m catriona-m marked this pull request as ready for review February 6, 2025 11:59

err = client.CreateOrUpdateThenPoll(ctx, *id, *model, backupvaults.DefaultCreateOrUpdateOperationOptions())
if err != nil {
return fmt.Errorf("creating Data Protection Backup Vault Customer Managed Key %s: %+v", *id, err)
Copy link
Member

Choose a reason for hiding this comment

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

Printing the ID will already contain the resource name, rephrasing this will remove the redundant information

Suggested change
return fmt.Errorf("creating Data Protection Backup Vault Customer Managed Key %s: %+v", *id, err)
return fmt.Errorf("creating Customer Managed Key for %s: %+v", *id, err)

Comment on lines 200 to 203
model := resp.Model
if model == nil {
return fmt.Errorf("retrieving %s: `model` is nil", *id)
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update this so it's consistent with how we do these nil checks in other update methods

Suggested change
model := resp.Model
if model == nil {
return fmt.Errorf("retrieving %s: `model` is nil", *id)
}
if resp.Model == nil {
return fmt.Errorf("retrieving %s: `model` is nil", *id)
}
if resp.Model.Properties == nil {
return fmt.Errorf("retrieving %s: `properties` is nil", *id)
}
model := resp.Model


err = client.CreateOrUpdateThenPoll(ctx, *id, *model, backupvaults.DefaultCreateOrUpdateOperationOptions())
if err != nil {
return fmt.Errorf("updating Data Protection Backup Vault Customer Managed Key for %s: %+v", *id, err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("updating Data Protection Backup Vault Customer Managed Key for %s: %+v", *id, err)
return fmt.Errorf("updating Customer Managed Key for %s: %+v", *id, err)

# azurerm_data_protection_backup_vault_customer_managed_key

Manages a Backup Vault Customer Managed Key.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a cautionary note here about how this can only be deleted/removed by deleting the parent resource?

* `create` - (Defaults to 30 minutes) Used when creating the Backup Vault Customer Managed Key.
* `read` - (Defaults to 5 minutes) Used when retrieving the Backup Vault Customer Managed Key.
* `update` - (Defaults to 30 minutes) Used when updating the Backup Vault Customer Managed Key.
* `delete` - (Defaults to 5 minutes) Used when deleting the Backup Vault Customer Managed Key.
Copy link
Member

Choose a reason for hiding this comment

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

I'm split about listing the delete here, since we can't delete it..

Copy link
Member Author

Choose a reason for hiding this comment

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

same, but leaning towards removing it now since we've added a note about the delete at the top, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me

stephybun
stephybun previously approved these changes Feb 13, 2025
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @catriona-m provided tests are passing LGTM 🏄‍♀️

@catriona-m
Copy link
Member Author

Thanks @catriona-m provided tests are passing LGTM 🏄‍♀️

Just made one small change after this, but the tests are passing 👍

@catriona-m catriona-m merged commit e4c8cef into main Feb 14, 2025
33 checks passed
@catriona-m catriona-m deleted the cm/dpvaultencryption branch February 14, 2025 12:15
@github-actions github-actions bot added this to the v4.20.0 milestone Feb 14, 2025
catriona-m added a commit that referenced this pull request Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add encryption block attribute to azurerm_data_protection_backup_vault
2 participants