Skip to content

Conversation

@celloza
Copy link
Contributor

@celloza celloza commented Aug 6, 2025

Description

The validation on retention periods utilising use_extended_retention do not function as expected.

Even if you specify use_extended_retention = true, the condition will still fail:

│ Error: Invalid value for variable
│ 
│   on main.tf line 137, in module "az_backup":
│  137:   blob_storage_backups = {
│  138:     imagebackups = {
│  139:       backup_name                  = "imagebackups"
│  140:       retention_period             = "P30D"
│  141:       backup_intervals             = ["R/2025-01-01T00:00:00+00:00/P1D"]
│  142:       operational_retention_period = "P7D"
│  143:       use_extended_retention       = true
│  144:       storage_account_id           = azurerm_storage_account.backup_target.id
│  145:       storage_account_containers   = [
│  146:         azurerm_storage_container.nhsapp_images_sc.name
│  147:       ]
│  148:     }
│  149:   }
│     ├────────────────
│     │ local.valid_retention_periods is tuple with 7 elements
│     │ var.blob_storage_backups is map of object with 1 element
│     │ var.use_extended_retention is false
│ 
│ Invalid retention period: valid periods are up to 7 days. If you require a
│ longer retention period then please set use_exetended_retention to true.
│ 
│ This was checked by the validation rule at
│ .terraform/modules/az_backup/infrastructure/variables.tf:79,3-13.

This PR fixes that by only evaluating the condition if use_extended_retention = false.

Type of change

Please check the relevant options:

🔲 New feature (a change which adds functionality)
✅ Bug fix (a change which fixes an issue)
🔲 Refactoring (code cleanup or optimisation)
🔲 Testing (new tests, or improvements to existing tests)
🔲 Pipelines (changes to pipelines and workflows)
🔲 Documentation (changes to documentation)
🔲 Other (something that's not listed here - please explain)

Checklist

Please check the relevant options:

✅ My code aligns with the style of this project
🔲 I have added comments in hard to understand areas
🔲 I have added tests that prove my change works
🔲 I have updated the documentation
🔲 If merging into main, I'm aware that the PR should be squash merged with a commit message that adheres to the semantic release format

Additional Information

N/A

@celloza celloza marked this pull request as ready for review August 12, 2025 07:21
@johncollinson2001 johncollinson2001 merged commit 8f6c8ed into main Dec 10, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants