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

New resource: mysql_global_variable #14

Merged
merged 3 commits into from
Jun 27, 2022

Conversation

borissavelev
Copy link

To have state for MySQL global configuration with IaC I'd like to introduce new resource: mysql_global_variable

To have state for MySQL global configuration with IaC I'd like to introduce new resource: mysql_global_variable
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.

My take on this:

  1. I wouldn't use it as the changes are not persisted and MySQL restart removes them. That said, it could still be used for some ad-hoc settings. As it has a chance to be used, I'm not explicitly against merging it.
  2. Please write tests. It's really easy for terraform - you can inspire yourself with mysql/resource_database_test.go. Something setting a (new) variable, changing it and "destroying it" (setting the default) would do it. I'm asking for this to be sure it works on all supported databases and there won't be critical issues I did not notice.
  3. Consider writing a documentation inside website subfolder.

To sum up, I have some notes about the code and I'd like to see a test. Once that is done and there are no new issues, I'm happy to merge your changes.

mysql/resource_global_variable.go Outdated Show resolved Hide resolved
mysql/resource_global_variable.go Outdated Show resolved Hide resolved
mysql/resource_global_variable.go Outdated Show resolved Hide resolved
@borissavelev
Copy link
Author

Ah, good point about persistence. I'm using this with tidb and all global variables are persistent there by default.

For mysql 8 there is an option to use persistent keyword but I need to test it.

Thank you! Fixes and tests are coming soon)

@borissavelev borissavelev requested a review from petoju June 25, 2022 20:22
@petoju petoju merged commit 4e47831 into petoju:master Jun 27, 2022
@petoju
Copy link
Owner

petoju commented Jun 27, 2022

Thanks for improving it and tests.

Merged.

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