search

petoju / terraform-provider-mysql

Terraform MySQL provider – unofficial fork
https://registry.terraform.io/providers/petoju/mysql
Mozilla Public License 2.0
65 stars 42 forks source link

Feature request: binlog retention #56

Closed cemdorst closed 2 months ago

cemdorst commented 1 year ago

Hi there,

first of all, thanks for the great work on this provider, it really impacts positively mysql community.

We have been using the provider alongside Ansible in order to get binlog retention period set. This is fine, but it doesn't really keep the state of binlog retention period. It would be very nice to have a resource to manage the binlog retention period to accomplish this task.

Here is the official aws documentation on how to do set retention period on a RDS mysql instance: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/mysql_rds_set_configuration.html

At Giphy, we have extended the provider to support binlog retention. We were using internally to test and published the modified version here: https://github.com/Giphy/terraform-provider-mysql/pull/1/files . We would be very happy if this goes to mainstream :-)

If you believe this will add value to the provider, I can open a PR with the changes and we can discuss from there.

Thank you!

References

petoju commented 1 year ago

Hi,

it looks possible, but I have some questions and remarks:

  1. Is there any way for a mere mortal to test it if something goes wrong with it? I mean, one can get a free tier RDS, but is there something better.
  2. Couldn't you do something like this? SET PERSIST expire_logs_days=10;. If it doesn't work, then ok... Even when I don't like when someone makes their own configs in a "compatible" DB, as long as it can be avoided.
  3. I'd call your resource in a way it would be obvious it's related to RDS.
  4. Would it be difficult for you to support also target delay? My idea would be to provide 1 resource to set any RDS configuration (if we're adding it) compared to 1 resource for 1 specific config.
  5. (I checked also https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/db_parameter_group - and it's not supported there - if AWS could make it available like that, it would be the best solution)

To sum up, I will likely accept PRs that solve this and try to address the points here. I wouldn't actively write it myself, but it seems you are handling this, so that's fine.

cemdorst commented 1 year ago

Hi, it looks possible, but I have some questions and remarks:

  1. Is there any way for a mere mortal to test it if something goes wrong with it? I mean, one can get a free tier RDS, but is there something better.

Unfortunately I am not aware of anything that could replace the RDS instance for this case.

  1. Couldn't you do something like this? SET PERSIST expire_logs_days=10;. If it doesn't work, then ok... Even when I don't like when someone makes their own configs in a "compatible" DB, as long as it can be avoided.

Typically RDS mysql doesn't allow you to set this parameter in this way. Connected as root you get an error like: mysql> SET PERSIST expire_logs_days=10; ERROR 1227 (42000): Access denied; you need (at least one of) the SUPER or SYSTEM_VARIABLES_ADMIN privilege(s) for this operation. I have seen some ugly workarounds for that, but I would do it as per AWS documentation https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/mysql_rds_set_configuration.html

  1. I'd call your resource in a way it would be obvious it's related to RDS.

Sure. mysql_rds_config would be fine?

  1. Would it be difficult for you to support also target delay? My idea would be to provide 1 resource to set any RDS configuration (if we're adding it) compared to 1 resource for 1 specific config.

It is possible. This will demand a big change on our work. Will see in the next days if I can come up with this new approach.

  1. (I checked also https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/db_parameter_group - and it's not supported there - if AWS could make it available like that, it would be the best solution)

I agree. Setting it up in the parameter group would be the best solution.

To sum up, I will likely accept PRs that solve this and try to address the points here. I wouldn't actively write it myself, but it seems you are handling this, so that's fine.

Cool, thanks for the reply. I will try to have the requested changes for the next week.

petoju commented 1 year ago

I have seen some ugly workarounds for that, but I would do it as per AWS documentation

Ok, looks good.

Sure. mysql_rds_config would be fine?

Yes, that's fine.

Cool, thanks for the reply. I will try to have the requested changes for the next week.

Also thanks for your work! If something is too difficult or unreasonable, feel free to come back to me.

petoju commented 2 months ago

Just going thru the old issues and this was fixed longer time ago with your fix. Thanks!