-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add PostgreSQLServerConfiguration managed resource #285
Add PostgreSQLServerConfiguration managed resource #285
Conversation
- Support for configuring PostgreSQLServer parameters - Fixes crossplane-contrib#281 Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
1d7a0a6
to
703c0c2
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.
Thank you @ulucinar for working on this challenging resource! I have a few comments but it mostly looks good.
// Delete deletes the given PostgreSQL Server Configuration | ||
func (c *PostgreSQLConfigurationClient) Delete(ctx context.Context, cr *azuredbv1beta1.PostgreSQLServerConfiguration) error { | ||
source := SourceSystemManaged | ||
return c.update(ctx, cr, &cr.Status.AtProvider.DefaultValue, &source) |
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 might be worth to have a code comment about why we call update.
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.
Done.
pkg/controller/database/postgresqlserverconfiguration/managed.go
Outdated
Show resolved
Hide resolved
pkg/controller/database/postgresqlserverconfiguration/managed.go
Outdated
Show resolved
Hide resolved
pkg/controller/database/postgresqlserverconfiguration/managed.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
a9c9b70
to
7e6a8b4
Compare
- Add tests Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
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.
Thank you @ulucinar !
Thanks @muvaf for the support. |
Description of your changes
Support for configuring PostgreSQLServer parameters. If the configuration has been applied successfully, the
PostgreSQLServerConfiguration
MR will have theReady
condition in its status.Setting the external-name annotation
When you run
az postgres server configuration list
to list available configurations for a psql instance, you will observe a JSON array of configuration objects similar to the following:My observation is that for a psql (server) configuration that has not been manipulated previously, the
postgresql.ConfigurationsClient.Get
will report a 404 and for the MR,postgresqlserverconfiguration/external.Create
will be called accordingly. However, if it has previously been manipulated, and even if it's set to its default value and itssource
is reset tosystem-default
, I have observed thatpostgresql.ConfigurationsClient.Get
returns the configuration object andpostgresqlserverconfiguration/external.Create
is thus not called. For this reason, we also set the external-name duringpostgresqlserverconfiguration/external.Observe
if it's not already set. The external name takes the following form:/subscriptions/<subscription id>/resourceGroups/<resource group name>/providers/Microsoft.DBforPostgreSQL/servers/<psql server name>/configurations/<configuration name>
Deletion Semantics
postgresql.ConfigurationsClient
does not have a delete method, probably because these configuration objects somewhat always exist for a psql instance (their sources being set tosystem-managed
), but as explained above, this behavior is not consistent. While deciding on what to do when aPostgreSQLServerConfiguration
MR is deleted, I took a look at how Terraform'sazurerm_postgresql_configuration
resource behaves, and as revealed by anaz postgres server configuration show
, it resets the configuration value to the default value but does not reset itssource
. I've chosen to implement a similar behavior of resetting to the default value but also itssource
is reset tosystem-managed
. As explained above, my observation is that the configuration object still exists in ARM and thus I had to observe thedeletionTimestamp
on the MR together with itssource
beingsystem-managed
and itsvalue
being equal to itsdefaultValue
for removing the finalizer on the MR.Fixes #281
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
I've manually configured an existing PostgreSQLServer instance's
max_wal_senders
parameter using aPostgreSQLServerConfiguration
MR.