Add label filter to list data sources #19

adriansuarez · 2024-02-16T04:24:10Z

Add label filter to databases and projects (plural) data sources, which causes the labelFilter query parameter to be injected with a comma-separated list of the label constraints when invoking GET.
Convert the filter block to a nested attribute. Exposing configuration as blocks seems to be a legacy feature and is discouraged in documentation for the Terraform plugin framework.
Add schema builder that eliminates boilerplate when creating list data sources.
Make some changes based on PR feedback for Use code generation for Terraform schemas and model structs #16.
Adding some test coverage of the attributes that were introduced by Use code generation for Terraform schemas and model structs #16 and of the label filtering.

sivanov-nuodb

Looks good.

sivanov-nuodb · 2024-02-16T12:20:49Z

internal/framework/schema_builder.go

@@ -0,0 +1,141 @@
+package framework


Nit: Missing copyright text.

sivanov-nuodb · 2024-02-16T12:47:07Z

internal/provider/databases_data_source.go

+	sb := framework.NewSchemaBuilder().WithDescription("Data source for listing NuoDB databases created using the DBaaS Control Plane")
+	sb.WithProjectScopeFilters("databases")
+	sb.WithProjectScopeList("database", "databases").WithNameAttribute("database")
+	return sb.Build()


Nit: Why not making this "one-liner"?

return framework.NewSchemaBuilder(). WithDescription("Data source for listing NuoDB databases created using the DBaaS Control Plane"). WithProjectScopeFilters("databases"). WithProjectScopeList("database", "databases"). WithNameAttribute("database"). Build()

It doesn't work. WithProjectScopeFilters() returns the *AttributeBuilder and not *SchemaBuilder, since it generates a nested attribute that we can attach attributes to. We would have return the parent to enable that, but then we would not be able to use WithOrganizationScopeFilters() inside of WithProjectScopeFilter() to minimize code duplication. When we have backups or other database scope resource, we may want to invoke WithProjectScopeFilter() inside of WithDatabaseScopeFilter(). Maybe it makes more sense to just duplicate the code in the builder to make it easier for the caller to use.

sivanov-nuodb · 2024-02-16T12:47:54Z

internal/framework/schema_builder.go

sivanov-nuodb · 2024-02-16T12:51:06Z

internal/provider/project_resource_test.go

+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "name", "proj"),
+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "sla", "dev"),
+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "tier", "n0.nano"),
+					// Labels should be removed due to explicit setting of labels={}


Does omitting the labels field have the same effect?

Omitting them does not have that effect, and whatever is in the state is retained.

I don't think this is the correct behavior: if a user deletes a label from their config, we should delete the label off of the resource. The fact that the user deleted the last label by explicitly defining the labels attribute as {}or omitting it should not change it. If you think that this is desirable behavior, it should be obvious in the documentation.

As a user, if I change my config, apply it, revert my commit, and apply the old state, I should return the system to its previous state. That is a core feature of infrastructure-as-code. Obviously, a user can expect that some changes are irreversible but we call those out in the docs and error out if the user tries it.

sivanov-nuodb · 2024-02-16T12:54:08Z

internal/provider/projects_data_source.go

 	}
-	projects, err := helper.GetProjects(ctx, client, organization, true)
+	projects, err := helper.GetProjects(ctx, client, organization, labelFilter, true)


Do we defer validation of the label filter to the REST API?

Yes, I think that is the most straightforward thing to do for now.

It is possible for us to add validators to the Terraform schema from the OpenAPI spec, based on pattern for example, which we can explore. But I think the error messages generated by the REST API are pretty clear.

For example, this is how an invalid product version is reported:

... nuodbaas_database.db: Modifying... [name=db] ╷ │ Error: Error updating database │ │ with nuodbaas_database.db, │ on example.tf line 19, in resource "nuodbaas_database" "db": │ 19: resource "nuodbaas_database" "db" { │ │ Error response from Control Plane service: status='HTTP 400 Bad Request', code=HTTP_ERROR, detail=[createDatabase.dbModel.properties.productVersion: does not match pattern │ '([1-9][0-9]*|[1-9][0-9]*\.[0-9]+|[1-9][0-9]*\.[0-9]+\.[0-9]+)([._-][a-z0-9._-]+)?': x.y] ╵

adriansuarez · 2024-02-16T13:50:05Z

@sivanov-nuodb I'm a little bit troubled by the e2e test failures. Maybe labels does not actually work with the Operator because labels are overridden somewhere?

sivanov-nuodb · 2024-02-16T13:53:55Z

@sivanov-nuodb I'm a little bit troubled by the e2e test failures. Maybe labels does not actually work with the Operator because labels are overridden somewhere?

The user-defined labels have special prefix injected by the REST service so I'm not sure how the operator can override them.
The labels present on the CR resource are just inherited by the underlying workloads and the operator doesn't do any special handling of them.

The operator just enforces that the workload labels are immutable after creation. The user can update the labels on the CR but the update won't be propagated to the workloads.

adriansuarez · 2024-02-16T15:16:08Z

The user-defined labels have special prefix injected by the REST service so I'm not sure how the operator can override them. The labels present on the CR resource are just inherited by the underlying workloads and the operator doesn't do any special handling of them.

The operator just enforces that the workload labels are immutable after creation. The user can update the labels on the CR but the update won't be propagated to the workloads.

Following up, the issue turned out to be in the REST service, which has a path rewrite that does not preserve query parameters. I am skipping the data sources tests in the e2e suite until that bug is fixed.

kontaras

I like the list filter logic. I have some cosmetic suggestions and additional test scenarios.

I don't like the list setting logic (not introduced in this change). We should at least document it so users are not caught off guard by it.

kontaras · 2024-02-16T16:00:18Z

internal/provider/project_resource_test.go

+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "labels.%", "1"),
+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "labels.key", "value"),
+					// product_version should be computed by the REST service
+					resource.TestCheckResourceAttrSet("nuodbaas_project.proj", "properties.product_version"),


This test is redundant because we immediately check that the version is 9.9.9.

Good catch.

kontaras · 2024-02-16T16:34:14Z

internal/provider/project_resource_test.go

+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "name", "proj"),
+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "sla", "dev"),
+					resource.TestCheckResourceAttr("nuodbaas_project.proj", "tier", "n0.nano"),
+					// Labels should be removed due to explicit setting of labels={}


I don't think this is the correct behavior: if a user deletes a label from their config, we should delete the label off of the resource. The fact that the user deleted the last label by explicitly defining the labels attribute as {}or omitting it should not change it. If you think that this is desirable behavior, it should be obvious in the documentation.

As a user, if I change my config, apply it, revert my commit, and apply the old state, I should return the system to its previous state. That is a core feature of infrastructure-as-code. Obviously, a user can expect that some changes are irreversible but we call those out in the docs and error out if the user tries it.

kontaras · 2024-02-16T16:54:55Z

internal/framework/schema_builder.go

+
+type NestedAttributeBuilder struct {
+	AttributeBuilder
+	parent *AttributeBuilder


Nit: parent is initialized and never used.

I can probably get rid of NestedAttributeBuilder since it adds no value. The idea with creating this was to allow the parent to be returned by chaining With...() calls, but if we call any method that is implemented by AttributeBuilder then we lose access to the parent. Golang does not have covariant return types that would enable chaining without losing the reference to original type.

kontaras · 2024-02-16T17:32:29Z

internal/framework/schema_builder.go

+	return ab.WithComputedStringAttribute("name", "The name of the "+typeName)
+}
+
+func (ab *AttributeBuilder) WithStringListAttribute(name, description string) *AttributeBuilder {


Nit: Since you are hard-coding the attribute as optional, consider making it more obvious by calling the method WithStringListArgument.

kontaras · 2024-02-16T17:33:35Z

internal/framework/schema_builder.go

+	parent *AttributeBuilder
+}
+
+func (ab *AttributeBuilder) WithNewNestedAttribute(name, description string) *NestedAttributeBuilder {


Nit: Again, make it obvious that this attribute is going to accept an argument structure.

kontaras · 2024-02-16T17:36:03Z

internal/framework/schema_builder.go

+	}
+}
+
+func (ab *AttributeBuilder) WithNewListNestedAttribute(name, description string) *NestedAttributeBuilder {


Nit: And this method can be more explicit about the fact that it is a read-only (output) attribute.

kontaras · 2024-02-16T17:37:49Z

internal/provider/database_data_source_test.go

+					// TODO(asz6): This is flaky because the mock controller that simulates the
+					// DBaaS operator sets the database as ready asynchronously with generating the
+					// CA certificate. This makes it is possible for the database to be marked as
+					// ready before the CA PEM is available, which is unrealistic. We can uncomment
+					// this once the mock controller is updated to have more realistic behavior.


Thank you for adding details

kontaras · 2024-02-16T17:44:54Z

internal/provider/databases_data_source_test.go

 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr(resourcePath, "databases.#", "1"),
 					resource.TestCheckResourceAttr(resourcePath, "databases.0.name", db1Name),
 					resource.TestCheckResourceAttr(resourcePath, "databases.0.organization", db1Org),
 					resource.TestCheckResourceAttr(resourcePath, "databases.0.project", db1Proj),
 				),
 			},
+			// Filter by project org1/proj1


Nit: We are not actually testing filtering by project, just that a filter can have a project field. This test would pass just as well if project was not sent to the CP.

kontaras · 2024-02-16T17:49:00Z

internal/provider/databases_data_source_test.go

+				// Skip if running end-to-end test because path rewrite used with Nginx is broken and does not preserve query parameters
+				// TODO: Remove once path rewrite is fixed
+				SkipFunc: func() (bool, error) { return IsE2eTest(), nil },
+			},


Test sending multiple filters.

- Add label filter to databases and projects (plural) data sources, which causes the labelFilter query parameter to be injected with a comma-separated list of the label constraints when invoking GET. - Convert the filter block to a nested attribute. Exposing configuration as blocks seems to be a legacy feature and is discouraged in documentation for the Terraform plugin framework. - Add schema builder that eliminates boilerplate when creating list data sources. - Make some changes based on PR feedback for #16. - Adding some test coverage of the attributes that were introduced by #16 and of the label filtering. NOTE: End-to-end testing exposed a bug in the REST service that affects 2.3.1 where the path rewrite done by the REST service was not preserving query parameters, which prevents label filtering from working.

adriansuarez requested review from kontaras and sivanov-nuodb February 16, 2024 04:24

sivanov-nuodb approved these changes Feb 16, 2024

View reviewed changes

kontaras reviewed Feb 16, 2024

View reviewed changes

adriansuarez force-pushed the asuarez/label-filter branch from b7cbf9c to 974657a Compare February 16, 2024 22:48

adriansuarez merged commit dc40693 into main Feb 16, 2024
1 of 2 checks passed

adriansuarez deleted the asuarez/label-filter branch February 16, 2024 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add label filter to list data sources #19

Add label filter to list data sources #19

adriansuarez commented Feb 16, 2024

sivanov-nuodb left a comment

sivanov-nuodb Feb 16, 2024

sivanov-nuodb Feb 16, 2024

adriansuarez Feb 16, 2024 •

edited

Loading

sivanov-nuodb Feb 16, 2024

sivanov-nuodb Feb 16, 2024

adriansuarez Feb 16, 2024

kontaras Feb 16, 2024

sivanov-nuodb Feb 16, 2024

adriansuarez Feb 16, 2024 •

edited

Loading

adriansuarez commented Feb 16, 2024

sivanov-nuodb commented Feb 16, 2024 •

edited

Loading

adriansuarez commented Feb 16, 2024 •

edited

Loading

kontaras left a comment

kontaras Feb 16, 2024

adriansuarez Feb 16, 2024

kontaras Feb 16, 2024

kontaras Feb 16, 2024

adriansuarez Feb 16, 2024

kontaras Feb 16, 2024

kontaras Feb 16, 2024

kontaras Feb 16, 2024

kontaras Feb 16, 2024

kontaras Feb 16, 2024

kontaras Feb 16, 2024

Add label filter to list data sources #19

Add label filter to list data sources #19

Conversation

adriansuarez commented Feb 16, 2024

sivanov-nuodb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansuarez Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansuarez Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

adriansuarez commented Feb 16, 2024

sivanov-nuodb commented Feb 16, 2024 • edited Loading

adriansuarez commented Feb 16, 2024 • edited Loading

kontaras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansuarez Feb 16, 2024 •

edited

Loading

adriansuarez Feb 16, 2024 •

edited

Loading

sivanov-nuodb commented Feb 16, 2024 •

edited

Loading

adriansuarez commented Feb 16, 2024 •

edited

Loading