Open DerekTBrown opened 7 months ago
Oh, I didn't know Example 1 has ever worked. Table is *
by default and that whole doesn't make sense.
That said, this is not documented, so people could use it in any way.
There are these things to solve:
mysql_grant
resource in case we decide to remove the old solution. I believe this is already like that, but further changes shouldn't break it.If we have solution, I'd review & merge the fixes.
@petoju Sorry for the delay. I ran into some additional things that rely on Example 1. Would it be possible to merge https://github.com/petoju/terraform-provider-mysql/pull/98 , which adds backwards compatibility?
@DerekTBrown I added some comments to find out what you think about some areas, but I don't have any serious issues with merging it.
@petoju After reviewing your comments, I realized that my prior PR (https://github.com/petoju/terraform-provider-mysql/pull/98) was going to make things way, way uglier (for instance, we could no have resource grants with no table, but with a procedure in the database field, ick). It is a much more involved change, but I have a PR for schematizing the various types of role grants, and adding support for procedures:
@petoju thanks for reviewing!
To simplify the logic, I think it makes sense to provide a first-class mysql_grant_procedure and mysql_grant_function resource. This would greatly simplify the amount of special-case logic, and provide a clearer interface for consumers.
Do you think it makes sense to introduce specific resource blocks, now that the "backend" separates between these?
@DerekTBrown I'd merge them if the code was reused where possible, but I wouldn't like to delete the old ones in the near future. Users of this provider got used to them and refactoring is a lot of work.
So, I just realized that between https://github.com/petoju/terraform-provider-mysql/pull/98 and https://github.com/petoju/terraform-provider-mysql/pull/102, I forgot to include tests for both Example 1 and Example 2. It looks like the new implementation has issues with Example 2, because it doesn't like a Procedure grant having a table:
...
# mysql_grant.test_procedure must be replaced
-/+ resource "mysql_grant" "test_procedure" {
~ database = "PROCEDURE tf-test-48.test_procedure" -> "PROCEDURE tf-test-48"
~ id = "jdoe-tf-test-48@%:`tf-test-48`" -> (known after apply)
~ table = "*" -> "test_procedure"
# (5 unchanged attributes hidden)
}
The simplicity gains of https://github.com/petoju/terraform-provider-mysql/pull/102 come from the fact that there is a 1:1 mapping between a SHOW GRANTS
row and the resource data (we effectively go from ResourceData
-> Grant -> ResourceData
). Allowing both modes of defining procedures creates a 2:1 mapping, which breaks this model.
Here is a fix:
Re-opening: So, we definitely made a step closer to this by having different structs, but we need a different resource to truly make the code cleaner.
Context
The deprecated Hashicorp Terraform provider supported role grants in a very clunky way:
It appears that some attempts have been made to preserve this functionality:
However, backwards compatibility (particularly Example 1) has been lost. Attempting to migrate directly from the upstream provider to the
petoju
one results in an error like the following:Moreover, this approach is incredibly hacky and hard to reason about. Here is my ugly attempt to provide backwards compatibility:
https://github.com/petoju/terraform-provider-mysql/pull/98/files
Proposal
To simplify the logic, I think it makes sense to provide a first-class
mysql_grant_procedure
andmysql_grant_function
resource. This would greatly simplify the amount of special-case logic, and provide a clearer interface for consumers.