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: Add Support for DB Instance Role Associations #132

Closed
wants to merge 23 commits into from

Conversation

korenyoni
Copy link
Member

@korenyoni korenyoni commented Dec 26, 2021

what

  • Add support for DB Instance Role Associations
  • Add BridgeCrew exceptions for errors stemming from this module parameterizing some resource attributes (false positives).
  • Expand Terratest tests.
  • Bump minimum supported Terraform version to 1.0.0 (see below).

why

  • Some RDS DB engines support different features depending on the engine. These should be supported by the module.
  • The comprehensive Terratest suite in examples/complete and test/src will only work with Terraform 1.0.0 and above, and otherwise will result in "count cannot be determined before apply" errors.

references

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

Bridgecrew has found infrastructure configuration errors in this PR ⬇️

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

⚠️   Due to 1babcf5 - WIP: initial commit. - 2 new errors were added

Change details

Error ID Change Path Resource
BC_AWS_IAM_60 Added /main.tf aws_db_instance.default
BC_AWS_GENERAL_46 Added /main.tf aws_db_instance.default

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

⚠️   Due to 4558d73 - Auto Format - 2 new errors were added

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_46 Added /main.tf aws_db_instance.default
BC_AWS_IAM_60 Added /main.tf aws_db_instance.default

@korenyoni korenyoni force-pushed the feat/role-associations branch 2 times, most recently from 7f26613 to e64bb5f Compare December 26, 2021 19:54
@korenyoni korenyoni force-pushed the feat/role-associations branch from 3160966 to f2bf4c1 Compare December 26, 2021 19:57
@korenyoni
Copy link
Member Author

/test all

@korenyoni
Copy link
Member Author

/test all

@korenyoni
Copy link
Member Author

/test all

@korenyoni
Copy link
Member Author

/test all

@korenyoni
Copy link
Member Author

/test all

@korenyoni korenyoni marked this pull request as ready for review December 26, 2021 22:41
@korenyoni korenyoni requested review from a team as code owners December 26, 2021 22:41
@korenyoni
Copy link
Member Author

/test all

1 similar comment
@korenyoni
Copy link
Member Author

/test all

nitrocode
nitrocode previously approved these changes Dec 27, 2021
@korenyoni
Copy link
Member Author

/test all

@korenyoni korenyoni requested a review from nitrocode December 27, 2021 15:58
versions.tf Outdated
@@ -1,14 +1,10 @@
terraform {
required_version = ">= 0.13.0"
required_version = ">= 1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Woah. This is the first time we're doing this I believe.

Copy link
Member

Choose a reason for hiding this comment

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

We still have engagements using older versions. I think we'll need another approval on this if this is needed. Could you explain why this is being bumped to 1.0 ?

Copy link
Member Author

@korenyoni korenyoni Dec 27, 2021

Choose a reason for hiding this comment

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

There's an issue with https://github.com/cloudposse/terraform-aws-s3-bucket/blob/6947cac37cdf809192f803d02cb87bce87dee35d/main.tf#L376 not being able to be determined when the module enabled=false

But yes, this is not ideal

Copy link
Member Author

Choose a reason for hiding this comment

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

I did explain it briefly in the why section of my PR description

The comprehensive Terratest suite in examples/complete and test/src will only work with Terraform 1.0.0 and above, and otherwise will result in "count cannot be determined before apply" errors.

@korenyoni
Copy link
Member Author

/test all

@korenyoni
Copy link
Member Author

/test all

@adamantike
Copy link

Bump! Is there anything blocking this PR from being merged, that we can help with?

@Gowiem Gowiem requested a review from nitrocode April 17, 2022 17:33
@Gowiem
Copy link
Member

Gowiem commented Apr 17, 2022

/test terratest

@Gowiem
Copy link
Member

Gowiem commented Apr 17, 2022

/test test/terratest

@mergify
Copy link

mergify bot commented Jun 17, 2022

This pull request is now in conflict. Could you fix it @korenyoni? 🙏


module "role" {
source = "cloudposse/iam-role/aws"
version = "0.14.0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = "0.14.0"
version = "0.16.2"

Comment on lines +116 to +118
#source = "cloudposse/s3-bucket/aws"
#version = "0.44.1"
# TODO: remove
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we set these ?

Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

Please see comments

@nitrocode
Copy link
Member

/test test/terratest

enabled = local.s3_integration_enabled

acl = "private"
policy = join("", data.aws_iam_policy_document.bucket_policy.*.json)
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be causing the failure

@hans-d hans-d closed this Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for RDS DB Instance role associations
6 participants