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

add feature Master Passwords via Secrets Manager #157

Merged
merged 11 commits into from
Nov 14, 2023
Merged

Conversation

ByJacob
Copy link
Contributor

@ByJacob ByJacob commented May 28, 2023

@ByJacob ByJacob requested review from a team as code owners May 28, 2023 15:16
@adamantike
Copy link

This would be really useful to have, and could potentially avoid the need for #118 for most users, while also not exposing the password in the state file. Can this be reviewed?

outputs.tf Outdated
@@ -47,3 +47,8 @@ output "resource_id" {
value = join("", aws_db_instance.default.*.resource_id)
description = "The RDS Resource ID of this instance."
}

output "master_user_secret_arn" {

Choose a reason for hiding this comment

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

I think exposing the entire master_user_secret object would be useful for users, and will simplify this logic by avoiding those local values:

output "master_user_secret" {
  value = one(aws_db_instance.default[*].master_user_secret)

@adamantike
Copy link

@joe-niland, @Gowiem, gentle ping. Can this be reviewed?
It will be really useful to have this option merged, as this new feature gains more traction.

@Gowiem
Copy link
Member

Gowiem commented Oct 31, 2023

/terratest

@Gowiem Gowiem self-requested a review October 31, 2023 14:12
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@ByJacob this looks good and I'm happy to provide another review after my couple of small requests. One larger issue is that this module hasn't been updated in a while, and that has resulted in our tests rotting and them using an old version of our VPC module that includes a deprecated argument (see screenshot). Can you update the VPC + Subnet module usages in these two locations to the latest versions and then we'll re-run tests?

  1. https://github.com/cloudposse/terraform-aws-rds/blob/main/examples/complete/main.tf#L5-L26
  2. https://github.com/cloudposse/terraform-aws-rds/blob/main/examples/mssql/main.tf#L5-L24

CleanShot 2023-10-31 at 08 24 03

outputs.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented Oct 31, 2023

@adamantike thanks for the ping! I've seen you do that on a few PRs and it's appreciated when there are good fixes + enhancements like this one that fall through the cracks. Though I think we'd all like to do better, there are so many modules that the squeaky wheels get the grease. That said, I believe Erik + team are always looking for more maintainers so if you're interested in helping us get things reviewed + merged, please reach out to me or Erik via Slack and we can likely make that happen.

@adamantike
Copy link

@ByJacob, if needed, I can tackle the VPC and subnet module upgrade in the examples, for you to rebase your changes after that is done.

@ByJacob
Copy link
Contributor Author

ByJacob commented Nov 7, 2023

Thanks for message @adamantike. I forgot about this PR. Changes are added.

@Gowiem
Copy link
Member

Gowiem commented Nov 7, 2023

/terratest

@ByJacob
Copy link
Contributor Author

ByJacob commented Nov 8, 2023

/terratest

@Gowiem I fixed tflint and Readme

@Gowiem
Copy link
Member

Gowiem commented Nov 8, 2023

/terratest

@Gowiem
Copy link
Member

Gowiem commented Nov 9, 2023

/terratest

@Gowiem
Copy link
Member

Gowiem commented Nov 9, 2023

@ByJacob Tests are failing on the following:
CleanShot 2023-11-09 at 10 53 42

@ByJacob
Copy link
Contributor Author

ByJacob commented Nov 14, 2023

@ByJacob Tests are failing on the following: CleanShot 2023-11-09 at 10 53 42

Id was change. I fix it and add identifier parameter

hashicorp/terraform-provider-aws@a7cb14b

@Gowiem
Copy link
Member

Gowiem commented Nov 14, 2023

/terratest

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

Solid work @ByJacob -- Thanks for working through the test failures!

@Gowiem Gowiem merged commit 5702cff into cloudposse:main Nov 14, 2023
10 checks passed
@ByJacob
Copy link
Contributor Author

ByJacob commented Nov 14, 2023

Solid work @ByJacob -- Thanks for working through the test failures!

It was enough to tighten the main branch, there it was fixed :D

@Gowiem
Copy link
Member

Gowiem commented Nov 14, 2023

@ByJacob release of your changes are blocked on some internal issue that I'm fixing in #164. Once that ships, a new release will be cut for v1.1.0

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.

3 participants