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

feat!: Change the default value of "monitoring_enable_managed_prometheus" var to null #2188

Merged

Conversation

legal90
Copy link
Contributor

@legal90 legal90 commented Nov 18, 2024

Fixes #1706

This PR is a continuation of #1715 by removing the condition for monitoring_config and turning it into a "static" block.

This PR is a continuation of #2155.
Here we change the default value of the variable monitoring_enable_managed_prometheus to null.

That is supposed to fix the calculation of local.logmon_config_is_set. Now it will return true not only when monitoring_enable_managed_prometheus is set to true, but also when it is set to false.

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody
Copy link
Collaborator

Hi @legal90!

From the CI test:

Error: googleapi: Error 400: Cannot specify logging_config or monitoring_config together with logging_service or monitoring_service.
Details:
[
  {
    "@type": "type.googleapis.com/google.rpc.RequestInfo",
    "requestId": "0xbb9f414f03695c18"
  }
]
, badRequest

  with module.example.module.gke.google_container_cluster.primary,
  on ../../../cluster.tf line 22, in resource "google_container_cluster" "primary":
  22: resource "google_container_cluster" "primary" {
}

@legal90
Copy link
Contributor Author

legal90 commented Nov 20, 2024

@apeabody Thank you! I will take a look at it soon.

… null

That will allow to properly identify whether that var is set (overridden by a caller) or not.
That's important for the value of `local.logmon_config_is_set`, which is then used in
dynamic block conditions.
@legal90 legal90 changed the title feat: Turn "monitoring_config" into a static block feat!: Change the default value of "monitoring_enable_managed_prometheus" var to null Nov 22, 2024
@legal90
Copy link
Contributor Author

legal90 commented Nov 22, 2024

@apeabody I rolled back my previous changes and implemented the fix using a different approach - by changing the default value of "monitoring_enable_managed_prometheus" var to null.

Could you please re-trigger the integration test?

@apeabody
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@apeabody apeabody 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 contribution @legal90!

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody apeabody merged commit 31a1619 into terraform-google-modules:master Nov 22, 2024
4 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.

monitoring_enable_managed_prometheus field not working in v27.0.0
2 participants