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

fix: Fix dependencies and implementation of the logging configuration support feature #9

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

schniber
Copy link
Contributor

Description

This PR addresses the following:
#7 : using the locals to reference the workspace id has as a consequence that terraform does not know the interdependencies between the workspace, the alert manager config and the rule namespace group.
#8 : this feature will allow to add the support of cloudwatch logging to the prometheus workspace.

Motivation and Context

This PR fixes the example and adds the support for the prometheus worksapce logging configuration.

see issues fixed above.

Breaking Changes

No

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

…on and rule group namespace + addition of logging configuration support
@schniber schniber changed the title fix dependencies and implementation of the logging configuration support feature fix: fix dependencies and implementation of the logging configuration support feature Oct 20, 2022
@schniber schniber changed the title fix: fix dependencies and implementation of the logging configuration support feature fix: Fix dependencies and implementation of the logging configuration support feature Oct 20, 2022
@schniber
Copy link
Contributor Author

Hello @bryantbiggs,

This PR is ready. Thanks in advance for your feedback.

Bests.

@@ -10,7 +6,15 @@ resource "aws_prometheus_workspace" "this" {
count = var.create && var.create_workspace ? 1 : 0

alias = var.workspace_alias
tags = var.tags

dynamic "logging_configuration" {
Copy link
Member

Choose a reason for hiding this comment

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

this should be:

dynamic "logging_configuration" {
    for_each = length(var.logging_configuration) > 0 ? [var.logging_configuration] : []

    content {
      log_group_arn = logging_configuration.value.log_group_arn
    }
  }

and the variables will need to be updated to match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in new commit.

Thanks a ton !

@@ -1,7 +1,3 @@
locals {
Copy link
Member

Choose a reason for hiding this comment

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

this can be fixed with:

locals {
  workspace_id = var.create && var.create_workspace ? aws_prometheus_workspace.this[0].id : var.workspace_id
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in new commit.

Thanks a ton !

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

awesome, thanks for the PR!

@bryantbiggs bryantbiggs merged commit 53fed8d into terraform-aws-modules:master Oct 20, 2022
antonbabenko pushed a commit that referenced this pull request Oct 20, 2022
### [2.2.1](v2.2.0...v2.2.1) (2022-10-20)

### Bug Fixes

* Fix dependencies and implementation of the logging configuration support feature ([#9](#9)) ([53fed8d](53fed8d))
@antonbabenko
Copy link
Member

This PR is included in version 2.2.1 🎉

@schniber
Copy link
Contributor Author

@bryantbiggs I think we need to adapt the example also.
since we now get an issue at the creation. we need to add ":*" for the logging configuration.

@bryantbiggs
Copy link
Member

oy, thats rather odd - looks like we do https://github.com/hashicorp/terraform-provider-aws/blob/2d2308a4872843b878a292dd2c95730e16a7a2e6/internal/service/amp/workspace_test.go#L324

@schniber
Copy link
Contributor Author

so when I added the ":*" the example is working back.
I think there's a bug with the v4.35.0 then.
what do you advise to do next ?
shall we fix the example ?

@bryantbiggs
Copy link
Member

ya, we can just update the example to match what the provider tests show with the addition of the trailing :*

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants