search

cockroachdb / terraform-provider-cockroach

Terraform provider for CockroachDB Cloud
Apache License 2.0
57 stars 12 forks source link

Add logexportconfig resource #81

Closed jenngeorge closed 1 year ago

jenngeorge commented 1 year ago

Add a new LogExportConfig resource, which manages the log export configuration for a cluster.

This change is Reviewable

jenngeorge commented 1 year ago

Thanks @erademacher ! Reviewable is not letting me publish comments at the moment, so responding here:

This looks great! So just to make sure I understand, EnableLogExport is declarative, right? So Create and Update are basically the same operation?

Right! This doc mentions that https://www.cockroachlabs.com/docs/cockroachcloud/export-logs.html?filters=gcp-log-export#update-an-existing-log-export-configuration, but I'll also update the API reference from "Create a Log Export configuration for a cluster" to "Create or update the Log Export configuration for a cluster".

                },
                "min_level": schema.StringAttribute{
                    Required:            true,

This probably shouldn't actually be required if it's allowed to be empty, like in the example.

Good catch! I'll ask about this while making the API update.

        resp.Diagnostics.AddError(
            "Error enabling LogExport",
            fmt.Sprintf("Could not enable LogExport: %v", formatAPIErrorMessage(err)),

nit: Mixing "LogExport" and "log export". Should probably standardize on "log export".

Good catch, fixed!

    minLevel := group.MinLevel.ValueString()
    clientGroup := client.LogExportGroup{
        Channels: &channels,

The fact that the client object here takes a bunch of pointers suggests that none of these fields are marked as required in the OpenAPI spec. Should they be? If so, we should probably make an API update before merging this change.

Yeah, it looks like they should probably be marked required. I'll check and make an API update!