Update logs pipeline to separate integration and customer pipelines #332
Conversation
ghost
added
the
tt810
changed the title
|
Hi @zippolyte, thanks for the review. I have addressed your comments. |
|
@zippolyte thanks for the review, I updated the last comment. Let me know if we are all good @nmuesch ! thank you guys! |
| } | ||
| return false, err | ||
| } | ||
| return ddPipeline.GetIsReadOnly(), nil |
There was a problem hiding this comment.
If the given ID exists but its a custom pipeline we return false? Should we instead print a message that this is a custom pipeline and should be created via the integration pipeline resource ?
There was a problem hiding this comment.
I notice terraform is calling this function for other types of instructions. If I return true for a custom pipeline, it would not be accurate.
| `Unprocessable Entity` with error message like `Cannot map pipelines to existing ones`, most likely the pipelines are | ||
| incompatible with the ones declared in your `datadog_logs_pipeline_order` resource. In this case, make | ||
| sure that no pipeline gets added or deleted via other method (for example, through API call or from Datadog Logs | ||
| configuration UI). |
There was a problem hiding this comment.
Is there anything we can do to print a more helpful message to the user? Maybe even just printing the list of pipelines and asking users to confirm they have everything in that list defined in their config?
There was a problem hiding this comment.
(That was what we discussed last time) I find is not very straightforward on API side to do so. Do you think it's enough just document it for now?
There was a problem hiding this comment.
Ahh ok. Can/do we provide a helpful error message in the code if this error is retrieved?
There was a problem hiding this comment.
I updated the method to do an extra call a GET to get the order from the API, and print it along with the order from resource? what do you think? (it's a extra call to the API, but maybe it worths it since otherwise, user probably gonna need to do that anyways).
There was a problem hiding this comment.
I also remove this troubleshooting paragraph.
|
Hi @zippolyte thanks for approving it. Do you mind to merge it (as I don't have this permission). |
integration pipline, rename the existing one tocustomer_pipline.pipline order