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

azurerm_backup_policy_vm support instant_restore_retention_days property #8822

Merged

Conversation

Lucretius
Copy link
Contributor

Resolves #8321

I've added validation to ensure the retention range is at least 1 (it is in days, and zero didn't seem to make sense as an input) but let me know if this is incorrect.

Copy link
Collaborator

@katbyte katbyte 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 the PR @Lucretius - this lookgs good, but i am curious if the API will take 0 as a value?

Other then that question this is good to merge!

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @Lucretius - upon giving this a once more over i think instant_restore_retention_days is a bit more clear. As such i've pushed a change to update it and am running tests

LGTM 👍

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Looks like we have a test failure, details on the line causing it i think

@jackofallops jackofallops modified the milestones: v2.32.0, v2.33.0 Oct 15, 2020
@jackofallops
Copy link
Member

Hi @Lucretius - I just re-ran the tests and we're still seeing the error above:

Error: Error creating/updating Azure Backup Protection Policy "acctest-201022083134290184" (Resource Group "acctestRG-backup-201022083134290184"): backup.ProtectionPoliciesClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="UserErrorInvalidRequestParameter" Message="Parameter InstantRpRetentionRangeInDays in request is invalid\r\nPlease provide correct value for parameter InstantRpRetentionRangeInDays"```

Apologies, I've not had chance to review, can you confirm your fix? 

@Lucretius
Copy link
Contributor Author

@jackofallops I think I am going to have to close this PR, I cannot seem to figure out how to pass the correct value in here. I've tried leaving it out, specifying it as nil, providing a default (the value is apparently defaulted to 2). I've tried updating the backup SDK to the latest version in case it was an SDK bug - nothing seems to work, the same error shows up.

@ghost ghost removed the waiting-response label Oct 28, 2020
@jackofallops
Copy link
Member

Hi @Lucretius - I've had a quick dig and found that the problem seems to have been there's a default in the service (2 in the region I tested, but I'm not convinced it will be guaranteed to be the same in all locations), so I've set the schema to Computed and tweaked the code around sending it. I have the tests running again as I type, and if they pass I think this is good to merge.

@jackofallops
Copy link
Member

Acceptance tests passing:
image

@jackofallops jackofallops changed the title Backup policy instant rp property azurerm_backup_policy_vm support instant_restore_retention_days property Oct 29, 2020
Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@jackofallops jackofallops merged commit 17f4bb6 into hashicorp:master Oct 29, 2020
jackofallops added a commit that referenced this pull request Oct 29, 2020
@ghost
Copy link

ghost commented Oct 29, 2020

This has been released in version 2.34.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.34.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Nov 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for setting Instant Restore Snapshot Retention in VM Backup Policy
4 participants