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

Add MySQLServerConfiguration managed resource #299

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

sergenyalcin
Copy link
Contributor

@sergenyalcin sergenyalcin commented Nov 3, 2021

Signed-off-by: sergenyalcin yalcinsergen97@gmail.com

Description of your changes

Fixes #286

Implementing MySQLServerConfiguration managed resource.

Support for configuring MySQLServer parameters. If the configuration has been applied successfully, the MySQLServerConfiguration MR will have the Ready condition in its status.

I would like to say a few things about the problem I encountered while working on this.

Valid configurations of the MySQL Server have been determined in server side and new ones cannot be created. Only existing valid configurations can be updated. Therefore, if the config cannot be found in the result of the get call (in Observe function), instead of returning nil, an error is returned and the error is reported in status conditions of the MySQLServerConfiguration managed resource.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • I have manually configured an existing MySQLServer instance's connect_timeout parameter using a MySQLServerConfiguration managed resource.
  • I have manually tried to configure an existing MySQLServer instance's invalid configuration parameter using a MySQLServerConfiguration managed resource. The invalid parameter could not be configured as expected. But we observed the error message in status conditions.
  • I have tested late-initialization functionality by adding a MySQLServerConfigration MR without value field. I observed that, this field set to actual value by controller.

@sergenyalcin sergenyalcin marked this pull request as ready for review November 5, 2021 08:15
@sergenyalcin sergenyalcin changed the title [WIP] Add MySQLServerConfiguration managed resource Add MySQLServerConfiguration managed resource Nov 5, 2021
@sergenyalcin
Copy link
Contributor Author

sergenyalcin commented Nov 5, 2021

The behavior I mentioned above (valid configurations are determined on the server side and new ones cannot be created) is the same for PostgreSql. In description of PostgreSQLServerConfiguration PR (#285), there is the following expression:

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 its source is reset to system-default, I have observed that postgresql.ConfigurationsClient.Get returns the configuration object and postgresqlserverconfiguration/external.Create is thus not called.

My observation for both MySql and PostgreSql does not agree with this observation. As I mentioned above, all valid configurations are defined on the server side and the Get call returns successfully for a valid configuration whether it is manipulated or not. So, if there is no error in the old observation, there may have been a behavior change in the Azure Sdk since then.

It should also be noted that there is no problem in the main workflow on the PostgreSql side. That is, there is no error in configuring a valid parameter (by creating a Managed Resource) or reverting the configured parameter (by deleting the Managed Resource). The improvement mentioned here is that when trying to configure an invalid parameter, the error encountered is reported in status.conditions (We do not see any errors now).

The improvement on the PostgreSql side is not included to this PR, as it is not the subject of this PR, and because the PostgreSql resources that were created in the past may be running with the old behavior.

Thanks to @ulucinar for his guidance on this task.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @sergenyalcin,
Thank you for working on this. I think there are no blockers for merging this PR, and I've left some comments for you to consider. Could you please also manually test the import case, i.e., no config value is specified in the MR to have it late-initialized? Could you please also check if we can increase coverage with some reasonable test cases?

apis/database/v1beta1/configuration_types.go Outdated Show resolved Hide resolved
apis/database/v1beta1/configuration_types.go Outdated Show resolved Hide resolved
pkg/clients/database/configuration/mysql.go Show resolved Hide resolved
pkg/clients/database/configuration/mysql.go Show resolved Hide resolved
pkg/clients/database/configuration/util.go Outdated Show resolved Hide resolved
pkg/controller/azure.go Outdated Show resolved Hide resolved
pkg/clients/database/configuration/mysql.go Show resolved Hide resolved
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you very much @sergenyalcin!

@ulucinar
Copy link
Collaborator

ulucinar commented Nov 18, 2021

Because we do not squash commits and preserve commit history, we should author a clean & descriptive commit history. @sergenyalcin could you please squash your commits?

Signed-off-by: sergenyalcin <yalcinsergen97@gmail.com>
@ulucinar ulucinar merged commit c182111 into crossplane-contrib:master Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MySQLServerConfiguration managed resource
2 participants