-
Notifications
You must be signed in to change notification settings - Fork 191
Adding authentication plugin support, mainly for AWSAuthenticationPlu… #26
Conversation
…gin use with Aurora in RDS, issue #16175
Thanks for the continued work on this, @doyouevensunbro. For some reason the Travis-CI build didn't trigger for this PR, and I'm not sure why... for this provider in particular we actually run the acceptance tests in Travis using the Travis-provided MySQL service, so we might need to tweak the settings for that in order to enable the test auth plugin. When I get a moment I'll get up to speed with how the Travis-CI MySQL service works and see what we need to do here. |
Instead of using the test plugin, I used the
Hope that helps. |
@apparentlymart - Any thoughts? This is an excellent feature to the provider. |
Just let me know if I need to do anything else. |
Hi again, @doyouevensunbro! I just wanted to check in and say that I've not forgotten about this but unfortunately I've not had an opportunity to properly look at this yet due to us being deep in a planning and design phase for Terraform Core. I do appreciate your work on this, and we'll get it reviewed and merged as soon as we can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @doyouevensunbro, sorry for the really late review here, but I have some changes that I'd like to see before we merge this. If you can look into these that would be great, otherwise we'd be happy to make the changes as we merge!
Thanks!
mysql/resource_user.go
Outdated
"password": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ConflictsWith: []string{"plaintext_password"}, | ||
Sensitive: true, | ||
Deprecated: "Please use plaintext_password instead", | ||
}, | ||
|
||
"auth": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be modified to auth_plugin
, and make to be a flat TypeString
value? I'm not too sure if we will ever be putting anything else in here - grants are already handled by mysql_grant
, and we already have separate fields for plaintext passwords. So having the extra layer here is not entirely useful as it stands right now.
The field should also be made to conflict with all other authentication fields (including password
as well, even though it's deprecated, it can still be used).
website/docs/r/user.html.markdown
Outdated
@@ -26,6 +26,16 @@ resource "mysql_user" "jdoe" { | |||
} | |||
``` | |||
|
|||
```hcl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a heading be added here denoting that this example is a an example using an authentication plugin?
Sure thing. Just got back into town and will make those changes. |
Fixing... |
For the tests to complete, the Travis CI needs to have the following run in MySQL:
|
Hey @doyouevensunbro, looks like there's still this comment to address. Once this is done I'll do another check and work on merging. Thanks! |
…d the docs to reflect the new auth changes.
Hey @doyouevensunbro, thanks for the updates - I want to make a couple of updates to the docs and fix Travis so all tests run but I can do that in master. Thanks for your work! |
No problem! |
…gin use with Aurora in RDS, issue #16175