Add backuppolicy resource and data sources #54

adriansuarez · 2024-05-22T14:33:00Z

This change adds the backuppolicy resource for managing backup policies via the NuoDB DBaaS REST API, the backuppolicy data source for obtaining the state of an existing backup policy, and the backuppolicies data source for listing existing backup policies.

Other changes:

Update the set of REST resources and models that we generate Golang code for using oapi-codegen, which exposed a bug in oapi-codegen (Enum constants not prefixed with type name oapi-codegen/oapi-codegen#1614). The fix-generated.sh script implements a workaround for that bug.
Strip ANSI color codes and remove special formatting and line-wrapping from Error and Warning messages to allow asserts to check for the presence of substrings.

This change adds the backuppolicy resource for managing backup policies via the NuoDB DBaaS REST API, the backuppolicy data source for obtaining the state of an existing backup policy, and the backuppolicies data source for listing existing backup policies. Other changes: - Update the set of REST resources and models that we generate Golang code for using oapi-codegen, which exposed a bug in oapi-codegen (oapi-codegen/oapi-codegen#1614). The fix-generated.sh script implements a workaround for that bug. - Strip ANSI color codes and remove special formatting and line-wrapping from Error and Warning messages to allow asserts to check for the presence of substrings.

adriansuarez · 2024-05-22T15:47:20Z

I need to do something about the examples tests, which try to invoke terraform apply on every data source. It doesn't seem easy to conditionalize them on whether the server supports the API, since they seem to run the examples in an opaque fashion.

The bigger issue is that this seems to have exposed an issue in listing of backup policies in the REST service itself. I will prioritize that.

I don't expect this PR to change in a big way, so it should be ready for review. I might have to shelve it because the REST service is broken for listing of backup policies and a fix needs to be deployed to the dev cluster.

EDIT: I pushed a change to skip the nuodbaas_backuppolicy and nuodbaas_backuppolicies directories when finding configs to apply, and there is a fix in the nuodb-control-plane repo for the access control issue having to do with listAccessible=true.

examples/resources/nuodbaas_backuppolicy/resource.tf

kontaras · 2024-05-22T15:23:57Z

examples/data-sources/nuodbaas_backuppolicies/data-source.tf

Please add tests in examples/examples_test.go for all the example plans.

What are we trying to accomplish with these tests? Just make sure that the examples are valid?

I think it would make more sense to invoke terraform validate on the configs, which hopefully does not make any request to the REST server, and this way we do not have to conditionalize execution on the resource and data source being supported by the server.

I made a change to only run terraform validate on the configs and rather than terraform apply.

I think that the purpose of running tests on the examples is to avoid publishing unusable configs in our docs and not to get test coverage of the provider, since we should get test coverage of the provider by writing explicit test cases rather than relying on the examples.

This change allows us to run the tests without a REST server.

Yes, the point is to make sure that our examples are correct.

I used plan instead of validate because it is more readily available in terraform-plugin-testing. Applying it to mock REST was an additional step to catch some errors that are not caught by our plugin parsing and validation logic.

Right. I could not find any way to invoke terraform validate using terraform-plugin-testing, so I migrated over to our own framework.

fix-generated.sh

internal/provider_test/backups_test.go

kontaras · 2024-05-22T20:57:57Z

internal/provider_test/suite_test.go

+	// Strip any ANSI color codes
+	if re, reErr := regexp.Compile(`(?m)\x1b\[[0-9;]*m`); reErr == nil {
+		out = re.ReplaceAll(out, []byte(""))
+	}
+	// Remove special formatting and line wrapping from "Error" and "Warning"
+	// messages so that paragraphs appear as one line and are searchable for
+	// the presence of expected substrings
+	if re, reErr := regexp.Compile(`(?m)\n│ ([[:graph:]])`); reErr == nil {
+		out = re.ReplaceAll(out, []byte(" $1"))
+	}


Thanks for fixing this

adriansuarez · 2024-05-23T12:41:41Z

The failures in the latest run of "External DBaaS" are due to quota violations in the shared environment, and there is still the bug in the server that I have to deploy the fix for.

I'll re-kick off the job once DBaaS dev cluster is in a usable state and has the fix for GET /backuppolicies/org?listAccessible=true.

sivanov-nuodb

Looks good.

sivanov-nuodb · 2024-05-23T13:05:23Z

examples/resources/nuodbaas_backuppolicy/resource.tf

@@ -0,0 +1,32 @@
+# A backup policy with minimal configuration


Nit: You could mention that this policy matches any database in organization named org and doesn't delete backups automatically.

sivanov-nuodb · 2024-05-23T13:08:09Z

fix-generated.sh

+
+# Patch types.go to fix names for enum constants. This is a workaround for https://github.com/deepmap/oapi-codegen/issues/1614.
+file=openapi/types.go
+sed 's/\([A-Za-z0-9_]*\)\(  *\)\(ProjectStatusModelState\) = "\1"/\3\1\2\3 = "\1"/' "$file" > "${file}.tmp"


Do we need the same fix for the DatabaseStatusModelState and BackupStatusModelState?

No, it is only happening for the ProjectStatusModelState.

sivanov-nuodb · 2024-05-23T13:17:19Z

internal/provider/backuppolicy/resource.go

+func (state *BackupPolicyResourceModel) Update(ctx context.Context, client openapi.ClientInterface, currentState framework.ResourceState) error {
+	// Fetch backup policy and get resourceVersion
+	latest := &BackupPolicyResourceModel{
+		Organization: state.Organization,
+		Name:         state.Name,
+	}
+	err := latest.Read(ctx, client)
+	if err != nil {
+		return err
+	}
+	for {
+		state.ResourceVersion = latest.ResourceVersion
+		resp, err := client.CreateBackupPolicy(ctx, state.Organization, state.Name, openapi.BackupPolicyModel(*state))
+		if err != nil {
+			return err
+		}
+		// Decode the response and check that there is no error
+		err = helper.ParseResponse(resp, nil)
+		if err == nil {
+			return nil
+		}
+		// If error is not retriable (code=CONCURRENT_UPDATE), fail fast
+		if apiError, ok := err.(*helper.ApiError); !ok || apiError.GetCode() != openapi.CONCURRENTUPDATE {
+			return err
+		}
+		// Re-fetch policy and get resourceVersion
+		err = latest.Read(ctx, client)
+		if err != nil {
+			return err
+		}
+	}
+}


Most of this code seems to be the same for all resources. Is it possible to factor out the duplicate code?

I remember looking into it in the past and deciding that it wasn't worth it because the database update required special processing for the DBA password anyway. Maybe it is worth it now that we have other resources that duplicate what the project resource does.

adriansuarez requested review from kontaras, sivanov-nuodb and agoelzer May 22, 2024 14:33

Fix e2e test and linter

1f39ad6

Skip backuppolicy and backuppolicies data sources

f3c70a5

kontaras reviewed May 22, 2024

View reviewed changes

adriansuarez added 4 commits May 22, 2024 21:48

Only validate examples, using custom test framework

8ce09cd

Move setting of retention

fb7592b

Check error for WalkDir()

e34c101

Update generated docs

a896129

sivanov-nuodb approved these changes May 23, 2024

View reviewed changes

kontaras approved these changes May 23, 2024

View reviewed changes

Add properties and labels to example

b335e9f

adriansuarez merged commit 8841acc into main May 23, 2024
7 checks passed

adriansuarez deleted the asuarez/backuppolicy-resource branch May 23, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backuppolicy resource and data sources #54

Add backuppolicy resource and data sources #54

adriansuarez commented May 22, 2024

adriansuarez commented May 22, 2024 •

edited

Loading

kontaras May 22, 2024

adriansuarez May 22, 2024

adriansuarez May 23, 2024 •

edited

Loading

kontaras May 23, 2024 •

edited

Loading

adriansuarez May 23, 2024

kontaras May 22, 2024

adriansuarez commented May 23, 2024

sivanov-nuodb left a comment

sivanov-nuodb May 23, 2024

sivanov-nuodb May 23, 2024

adriansuarez May 23, 2024

sivanov-nuodb May 23, 2024

adriansuarez May 23, 2024

		@@ -0,0 +1,32 @@
		# A backup policy with minimal configuration

Add backuppolicy resource and data sources #54

Add backuppolicy resource and data sources #54

Conversation

adriansuarez commented May 22, 2024

adriansuarez commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansuarez May 23, 2024 • edited Loading

Choose a reason for hiding this comment

kontaras May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansuarez commented May 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansuarez commented May 22, 2024 •

edited

Loading

adriansuarez May 23, 2024 •

edited

Loading

kontaras May 23, 2024 •

edited

Loading