-
Notifications
You must be signed in to change notification settings - Fork 127
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
Rdb: Allow RDB instance upgrade to HA without replacing the resource #518
Conversation
@@ -52,11 +51,14 @@ func resourceScalewayRdbInstanceBeta() *schema.Resource { | |||
"user_name": { | |||
Type: schema.TypeString, | |||
Required: true, | |||
ForceNew: true, |
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.
imho we should add a separate user resource, and if db creation with no user is not on the table, we should at least handle the update here, not a new resource
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.
Agreed that splitting the user to a separate resource makes more sens in TF. This would imply allowing the creation of the database without any user in the API or silently deleting it in the TF create
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.
I am planning on adding a separate user resource in an upcoming PR.
I am concerned about allowing user changes in the instance resource as it could potentially conflict with definition in the user resource.
For example in this template definition (after adding a user object):
resource scaleway_rdb_instance_beta db {
name = "test-terraform"
node_type = "db-dev-m"
engine = "PostgreSQL-11"
is_ha_cluster = true
user_name = "admin"
password = "azerty"
}
resource scaleway_rdb_user_beta db_admin {
instance_id = scaleway_rdb_instance_beta.db.id
name = "toto"
password = "R34lP4sSw#Rd"
is_admin = true
}
We don’t want to have a confusion on which password is used, so I want to set the expectation that the password in the user definition "always win", which is only possible if we don’t allow updates on the instance resource. As such the user password is always updated to the user object definition after the instance is created.
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.
This is the PR to add a specific resource for database users #520
@@ -39,7 +39,6 @@ func resourceScalewayRdbInstanceBeta() *schema.Resource { | |||
"is_ha_cluster": { |
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.
This is out of this MR but when reading it this field name seems strange, IMHO enable_ha_cluster
would make more sense don't you think ?
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.
It would make more sense indeed. is_ha_cluster is what is being used currently in the CreateInstance API.
So I am balancing using a better name vs keeping the attribute name in sync with the API and dealing with renaming an attribute without breaking compatibility.
I am more inclined to keep it as is for now, what do you think?
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.
IMO it makes more sense to write enable_ha_cluster = true
in the HCL:is_ha_cluster
is the read only name of the property through the API, whereas enable_ha_cluster
is an actionnable attribute in the terraform config.
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.
Sounds good, I'll update this in a separate PR.
@@ -253,7 +268,26 @@ func resourceScalewayRdbInstanceBetaUpdate(d *schema.ResourceData, m interface{} | |||
if err != nil { | |||
return err | |||
} | |||
|
|||
// Wait for the instance to settle after upgrading | |||
time.Sleep(30 * time.Second) |
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.
What issue did you encounter?
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.
This is an issue on RDB side. I will remove this when it is resolved.
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.
LGTM
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.
LGTM
Thanks for your contribution! @paulez 🙏 |
This is to allow converting an RDB instance to HA without recreating the resource.
This PR also contains minor fixes to the user_name and password fields which cannot be modified using the RDB instance API.
I plan on creating dedicated resources to manage users and databases in an upcoming PR.
To test I modified the acceptance test to also include an HA upgrade. This change will increase the test runtime by about 10 minutes, we can revert it if that’s too much of a time penalty.
The whole test suite timed-out because of an unrelated issue, so I ran the Rdb acceptance tests only with the following: