-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(resource): implement managed database resource types #187
Conversation
c2bd158
to
4904bae
Compare
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.
Generally speaking looks good. Left some comments and questions, but no real blockers.
The one thing that is imo missing is some documentation update.
Type: schema.TypeString, | ||
Computed: true, | ||
Optional: true, | ||
ValidateDiagFunc: func(i interface{}, path cty.Path) diag.Diagnostics { |
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.
Perhaps this could be replaced with a function from, validation
package?
ValidateFunc: validation.StringInSlice([]string{"monday", "tuesday" ...}, false)
Zone: d.Get("zone").(string), | ||
} | ||
|
||
if d.HasChange("properties.0") { |
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.
Perhaps I'm wrong here, but my understanding so far was that create
function is called only once for resource. If that's the case - what is the point of checking for changes here?
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.
yes could be. Anyway, the point is to check if custom properties are requested.
return diag.FromErr(fmt.Errorf("cannot create a logical database while managed database %v (%v) is powered off", serviceDetails.Name, serviceID)) | ||
} | ||
|
||
if d.HasChanges("character_set", "collation") && serviceDetails.Type != upcloud.ManagedDatabaseServiceTypePostgreSQL { |
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.
Again, not really sure about checking for changes in create
function. Wouldn't d.GetOk
be more suitable here?
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 don't know. AFAIK HasChanges calls GetOk internally.
However, the check is valid as those parameters are only valid for postgres.
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.
HasChanges
compare the previous value with the new one, GetOk
only checks if it's existing and not a zero-value, no?
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.
@BachirKhiati as said, I'm not entirely sure, the stuff isn't very well documented.
Optional: true, | ||
Computed: true, | ||
ForceNew: true, | ||
ValidateDiagFunc: validateManagedDatabaseLocale, |
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'm not sure if you need separate function for this, I think something like this would also work:
ValidateFunc: validation.StringMatch(regexp.MustCompile(`someregexp`), "some error msg")
Sensitive: true, | ||
Computed: true, | ||
Optional: true, | ||
ValidateDiagFunc: func(i interface{}, path cty.Path) diag.Diagnostics { |
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 probably be also handled by validation.StringLenBetween
function
return diag.FromErr(err) | ||
} | ||
|
||
log.Printf("[DEBUG] managed database user %v/%v (%v/%v) read", |
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.
Same question about this debug log as above
Wait managed database to be fully created and running before shutdown.
Wait managed database to be running and service name to be propagated before continuing execution. Add UpCloud Managed Databases to CHANGELOG.
Maintenance window properties were ignored when creating instances and also in updates if only one of two properties (maintenance_window_dow, maintenance_window_time) were changed.
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
This patch adds initial support for our Managed Databases. Moreover, we switch to importing the major >1 version of UpCloud GO SDK properly.