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 firehose policy management #80

Closed

Conversation

badcure
Copy link

@badcure badcure commented Sep 4, 2023

Description

There is an issue in terraform where using aws_iam_role_policy and inline_policy in the role itself causes an endless update. Deploying once will add the policy, deploying again will remove it.

This occurred with the following module setup(modified slightly):

module "cloudwatch_firehose_coralogix" {
  source             = "github.com/coralogix/terraform-coralogix-aws//modules/firehose"
  firehose_stream    = "coralogix-metrics"
  private_key        = data.aws_ssm_parameter.coralogix_api_key.value
  coralogix_region   = "us2"
  user_supplied_tags = module.label.user_supplied_labels
}

This fix will now allow each deploy from terraform to be unmodified.

Also fixing an issue with tags_all, the tags were not applied with the empty string from custom_endpoint when no custom domain was passed. For the time being, I put _default_ as the value.

How Has This Been Tested?

  1. Deploy the changes using Terraform.
  2. Deployed Terraform again and ensured there were no changes.

Checklist:

  • I have updated the relevant example in the examples directory for the module.
  • I have updated the relevant test file for the module.
  • This change does not affect module (e.g. it's readme file change)

Copy link
Contributor

@ryantanjunming ryantanjunming left a comment

Choose a reason for hiding this comment

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

Made my own changes, thanks

@ryantanjunming
Copy link
Contributor

There are some issues passing the test, copying over the code and recreating the PR here: #83

Will close this, thanks @badcure

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