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 Amazon RDS MySQL config #60

Merged
merged 19 commits into from
Feb 23, 2023

Conversation

cemdorst
Copy link

@cemdorst cemdorst commented Feb 1, 2023

Implements a new resource called mysql_rds_config that handles Amazons RDS MySQL config parameters that otherwise could only be configured using a local-exec approach. Discussion here #56

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Feat binlog retention

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
docs: add mysql_retention_period docs for terraform registry

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* feat: replace binlog resource for RDS config resource

* chore: add mysq_rds_config read, update and import acctest
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Show resolved Hide resolved
GNUmakefile Outdated Show resolved Hide resolved
@petoju
Copy link
Owner

petoju commented Feb 1, 2023

Thanks for the PR. There are some things to resolve, but generally, it looks good.

@cemdorst
Copy link
Author

cemdorst commented Feb 1, 2023

Thanks for the PR. There are some things to resolve, but generally, it looks good.

Thanks for considering this contribution and for you review. I think I have the requested changes fixed by 0dc96e0 . Let me know if looks good.

mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Show resolved Hide resolved
mysql/resource_rds_config_test.go Outdated Show resolved Hide resolved
@cemdorst cemdorst requested a review from petoju February 6, 2023 19:20
Copy link
Owner

@petoju petoju left a comment

Choose a reason for hiding this comment

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

Ok, we're getting there.
Once you address my comments, I'll spend my 10 cents to get RDS to run tests on that. Hopefully that will work.

mysql/resource_rds_config_test.go Show resolved Hide resolved
mysql/resource_rds_config.go Show resolved Hide resolved
mysql/resource_rds_config_test.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Outdated Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
@cemdorst
Copy link
Author

cemdorst commented Feb 9, 2023

Ok, we're getting there. Once you address my comments, I'll spend my 10 cents to get RDS to run tests on that. Hopefully that will work.

I think I have all addressed. My comment about the "+rds" or empty string got lost: I thought about creating a generic function that would return metadata do the Version (https://pkg.go.dev/github.com/hashicorp/go-version#section-readme). Possibly could be used by serverVersion or serverVersionString.

Copy link
Owner

@petoju petoju left a comment

Choose a reason for hiding this comment

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

Please rebase against my latest master and really try running the tests, if you can. Because some tests do not run as they should.

You can skip RDS in some of the tests - it makes sense.

mysql/resource_rds_config_test.go Show resolved Hide resolved
mysql/resource_rds_config.go Outdated Show resolved Hide resolved
mysql/resource_rds_config_test.go Outdated Show resolved Hide resolved
Copy link
Owner

@petoju petoju 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 your changes!

@petoju petoju merged commit 49e42bf into petoju:master Feb 23, 2023
@petoju
Copy link
Owner

petoju commented Feb 23, 2023

I release provider version 3.0.30 that includes this PR.

@cemdorst
Copy link
Author

I release provider version 3.0.30 that includes this PR.

That's great news!! I am very happy to contribute :-)

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.

None yet

2 participants