Skip to content

Use explicit flag for diagnostic settings to make deployment of diagnostic settings deterministic#62

Closed
celloza wants to merge 0 commit intoNHSDigital:mainfrom
celloza:main
Closed

Use explicit flag for diagnostic settings to make deployment of diagnostic settings deterministic#62
celloza wants to merge 0 commit intoNHSDigital:mainfrom
celloza:main

Conversation

@celloza
Copy link
Copy Markdown
Contributor

@celloza celloza commented Jul 28, 2025

Description

Fixes #61

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

Please provide any additional information or context related to this pull request.

@celloza celloza changed the title Use null string instead of length to determine whether diagnostic settings should be created Use explicit flag for diagnostic settings to make deployment of diagnostic settings deterministic Jul 29, 2025
@celloza celloza force-pushed the main branch 3 times, most recently from db8f600 to b429ce3 Compare July 29, 2025 15:39
Comment thread infrastructure/variables.tf Outdated
type = bool
default = false

validation {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, can we add some tests to assert the validation rule?

Here's an example - setup a funky scenario, then add this variable to the expect_failures block:

@johncollinson2001
Copy link
Copy Markdown
Contributor

We'll need to update this e2e test to take account of the new config:

"log_analytics_workspace_id": *externalResources.LogAnalyticsWorkspace.ID,

@johncollinson2001
Copy link
Copy Markdown
Contributor

Also an update needed to this terraform test:

run "configure_vault_diagnostics_when_enabled" {

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.

Using length for determining whether Log Analytics Workspace is created will always result in non-deterministic error

2 participants