[datadog_observability_pipeline] Add processor groups instead of standalone processors#3346
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Everything looks good to me, especially with the updated tests.
Still, this is a lot to review (and I'm sure a lot to generate on your side) so it's hard to ensure no regression was introduced.
It could be really worthwhile to update the contribution process doc to reference the new additions/ enrich the LLM prompt used when adding/updating a single component
270f5ad to
1ac6d3e
Compare
| NestedObject: schema.NestedBlockObject{ | ||
| Attributes: map[string]schema.Attribute{ | ||
| "method": schema.StringAttribute{ | ||
| Required: true, |
1ac6d3e to
112f598
Compare
brett0000FF
left a comment
There was a problem hiding this comment.
LGTM! Just a few tiny nits on the docs.
20agbekodo
left a comment
There was a problem hiding this comment.
Nice! Indeed it's a lot to review but the tests bring confidence.
I also see you changed the confluence doc on how to contribute.
Is it possible to add a test to expect an error when not filling a newly required nested field?
In general yes, but this would require adding quite a few tests to basically double-check whether the field is marked as Required or not... I'm hoping we could find a way to generate different pipeline configs and validating them outside the standard TF testing framework for more thorough testing. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Replace standalone processors with processor groups.
Before:
After:
Important context:
processorsis replaced with the list ofprocessor_group's, which have individualprocessorblocks inside. Bothprocessor_groupandprocessorblocks haveid,enabledandinclude;processorblocks don't haveinputsanymore, only processor groups;processorwrappers are required to maintain the order of processors(TF tracks the order only for the blocks of the same type). SeeTestAccDatadogObservabilityPipeline_multipleProcessorGroupsas an example test with reordering.ListNestedBlockwith the size validation(<=1), because of the way TF handles optional blocks. If we useSingleNestedBlock(which makes more sense) for an optional block, which contains any required field, TF considers this block as required. This is a recommended workaround(see Disallow use ofSingleNestedBlockin resource schemas hashicorp/terraform-provider-aws#35813 for example).Replacing sources, destinations with source, destination
As in the example above, we have replaced single
sourcesanddestinationsblocks with multiplesourceanddestinationblocks. This approach follows the TF convention for list blocks and lets us share common fields(id, inputs) at the component level.Additional breaking changes combined together to avoid further breaking PRs
ListNestedBlock's are renamed from plural forms to singular ones:remaps->remap,variables->variableetc. to follow the TF convention for list blocks;SingleNestedBlocks replaced withListNestedBlocks to make the TF schema more robust, future-proof and eliminate potential breaking changes related to required/optional blocks and fields. See Disallow use ofSingleNestedBlockin resource schemas hashicorp/terraform-provider-aws#35813 as an example of the same approach.Suggested migration flow for existing resources
If you need to make immediate changes, you can safely downgrade your Datadog Terraform provider to
3.83.0and make updates as before.idfield from the imported configuration.