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

feat(resource): implement managed database resource types #187

Merged
merged 8 commits into from
Jan 13, 2022

Conversation

aakso
Copy link
Member

@aakso aakso commented Oct 22, 2021

This patch adds initial support for our Managed Databases. Moreover, we switch to importing the major >1 version of UpCloud GO SDK properly.

Copy link
Contributor

@ka-myl ka-myl left a 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 {
Copy link
Contributor

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") {
Copy link
Contributor

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?

Copy link
Member Author

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.

upcloud/resource_upcloud_managed_database.go Outdated Show resolved Hide resolved
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 {
Copy link
Contributor

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?

Copy link
Member Author

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.

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, GetOkonly checks if it's existing and not a zero-value, no?

Copy link
Member Author

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,
Copy link
Contributor

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 {
Copy link
Contributor

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",
Copy link
Contributor

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.
@peknur peknur requested review from BachirKhiati and ka-myl and removed request for ajmyyra December 28, 2021 08:43
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.
Copy link
Contributor

@ka-myl ka-myl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peknur peknur merged commit 2fcc770 into master Jan 13, 2022
@peknur peknur deleted the feat/managed-databases branch January 13, 2022 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants