-
Notifications
You must be signed in to change notification settings - Fork 11
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 logexportconfig resource #81
Conversation
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.
This looks great! So just to make sure I understand, EnableLogExport
is declarative, right? So Create and Update are basically the same operation?
Reviewed 3 of 5 files at r1.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @jenngeorge)
internal/provider/log_export_config_resource.go
line 81 at r1 (raw file):
}, "min_level": schema.StringAttribute{ Required: true,
This probably shouldn't actually be required if it's allowed to be empty, like in the example.
internal/provider/log_export_config_resource.go
line 181 at r1 (raw file):
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".
internal/provider/log_export_config_resource.go
line 210 at r1 (raw file):
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.
cf32faa
to
cfb8498
Compare
Thanks @erademacher ! Reviewable is not letting me publish comments at the moment, so responding here:
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".
Good catch! I'll ask about this while making the API update.
Good catch, fixed!
Yeah, it looks like they should probably be marked required. I'll check and make an API update! |
ea2e6c1
to
94485aa
Compare
4b64c30
to
65e2ffa
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.
Here's a crl google doc with manual testing examples . It includes the AWS and GCP workflow examples, plus various create / update / delete attempts that build off the examples.
Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @erademacher)
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.
Reviewed 3 of 7 files at r3, 3 of 6 files at r4, 4 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jenngeorge)
internal/provider/log_export_config_resource.go
line 212 at r6 (raw file):
var groups []LogExportGroup if len(spec.GetGroups()) != 0 { groups = make([]LogExportGroup, 0, len(spec.GetGroups()))
super nit: There probably aren't enough items in these slices for it to make a big difference, but if no items are getting filtered out, it's generally more efficient to set the length to the right size (instead of the capacity) and use the index instead of append
.
Add a new LogExportConfig resource, which manages the log export configuration for a cluster.
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.
TFTR! Recent pushes include cloud provider compatibility checks that were present during manual testing but I had failed to push, rebasing, and indexing instead of appending.
Reviewable status: 4 of 10 files reviewed, 1 unresolved discussion (waiting on @erademacher)
internal/provider/log_export_config_resource.go
line 212 at r6 (raw file):
Previously, erademacher (Evan Rademacher) wrote…
super nit: There probably aren't enough items in these slices for it to make a big difference, but if no items are getting filtered out, it's generally more efficient to set the length to the right size (instead of the capacity) and use the index instead of
append
.
Good catch, fixed!
Add a new LogExportConfig resource, which manages
the log export configuration for a cluster.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)