-
Notifications
You must be signed in to change notification settings - Fork 389
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
Support Logs Index API #326
Conversation
tt810
commented
Sep 25, 2019
- Resource for index and tests
- Resource for index order and tests
- Documentation
|
||
## Example Usage | ||
|
||
Create a Datadog logs pipeline order resource |
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.
Can we put this as a comment in the sample code rather than up here?
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.
I was trying to make it look similar to logs_pipeline
. Do you think I should update both of them (pipeline and index)?
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.
Hmm might be best to update both!
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.
Yup, let's either put it in as a comment in the sample code or just remove it altogether (I don't think it's adding too much value TBH).
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.
Sounds good. I will remove this line for both index_order and pipeline_order.
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.
some small edits to documentation
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.
Hey, thanks for another awesome PR! This looks real good, I just left couple minor comments inline to request some improvements.
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.
LGTM now. Great work 👍
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.
Code looks good, I only left two comments to add comments, one question and one nitpick. Feel free to address them but nothing was blocking a merge 👍
return nil | ||
} | ||
|
||
func resourceDatadogLogsIndexOrderExists(d *schema.ResourceData, meta interface{}) (bool, error) { |
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 have been nice to explain with a comment why we always return true here
return nil | ||
} | ||
|
||
func resourceDatadogLogsIndexOrderDelete(d *schema.ResourceData, meta interface{}) error { |
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.
Same, a comment on why we don't need the delete method would have been nice.
return resourceDatadogLogsIndexRead(d, meta) | ||
} | ||
|
||
func resourceDatadogLogsIndexDelete(d *schema.ResourceData, meta interface{}) error { |
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.
Why can't we delete an index ?
return ddEFilters | ||
} | ||
|
||
func buildDatadogExclusionFilter(tfEFilter map[string]interface{}) datadog.ExclusionFilter { |
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.
nits: as a general remark, I think tfEFilter
is not really explicit and I'd rather use full name, e.g. tfExclusionFiler
. Same for tfFs
.