-
Notifications
You must be signed in to change notification settings - Fork 122
Add resource to manage ML anomaly detection jobs #1329
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
base: main
Are you sure you want to change the base?
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.
Looks good! Just a few questions.
), | ||
}, | ||
{ | ||
Config: testAccResourceAnomalyDetectionJobComprehensive(jobID), |
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.
Would it be worth separating into a basic create / update and "comprehensive" create / update test so we get coverage for things like updating the minimal example and creating the comprehensive example?
} | ||
|
||
// Convert API response back to TF model | ||
diags.Append(job.fromAPIModel(ctx, &response.Jobs[0])...) |
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.
Would it be worth adding an error diag here if there is more than one job in response.Jobs
? This would probably only be future proofing against the read API call being updated without the corresponding change here.
stringplanmodifier.RequiresReplace(), | ||
}, | ||
Validators: []validator.String{ | ||
stringvalidator.LengthBetween(1, 64), |
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.
Is this really capped at 64 characters? https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-ml-put-job#operation-ml-put-job-job_id
I didn't see this in the docs
Attributes: map[string]schema.Attribute{ | ||
"bucket_span": schema.StringAttribute{ | ||
MarkdownDescription: "The size of the interval that the analysis is aggregated into, typically between 15m and 1h. If the anomaly detector is expecting to see data at near real-time frequency, then the bucket_span should be set to a value around 10 times the time between ingested documents. For example, if data comes every second, bucket_span should be 10s; if data comes every 5 minutes, bucket_span should be 50m. For sparse or batch data, use larger bucket_span values.", | ||
Required: true, |
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.
Is this actually required? In the request body docs it makes it seem not required: https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-ml-put-job#operation-ml-put-job-body-application-json
}, | ||
NestedObject: schema.NestedAttributeObject{ | ||
Attributes: map[string]schema.Attribute{ | ||
"function": schema.StringAttribute{ |
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.
Is this actually required?
Optional: true, | ||
Computed: true, | ||
Validators: []validator.String{ | ||
// TODO: TB If only json is supported then why are we allowing delimited? |
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.
Leftover TODO
Fixes #520
This resource only manages the existence of an anomaly detection job, it does not manage the state (open/closed). I'm planning on adding another resource to purely manage that state, it's an independent API. IMO it's simpler for practitioners if these are separate resources altogether.