monitor_search is not yet supported #358
Conversation
ghost
added
| } | ||
| ``` | ||
|
|
||
| * `monitor` type SLOs: | ||
| * `monitor_ids` - (Optional) A list of numeric monitor IDs for which to use as SLIs. Their tags will be auto-imported into `monitor_tags` field in the API resource. At least 1 of `monitor_ids` or `monitor_search` must be provided. | ||
| * `monitor_search` - (Optional) The monitor query search used on the monitor search API to add monitor_ids by searching. Their tags will be auto-imported into `monitor_tags` field in the API resource. At least 1 of `monitor_ids` or `monitor_search` must be provided. | ||
| * `monitor_ids` - (Optional) A list of numeric monitor IDs for which to use as SLIs. Their tags will be auto-imported into `monitor_tags` field in the API resource. |
There was a problem hiding this comment.
If one of monitor_ids and monitor_search must be provided and we are hiding monitor_search, shouldn't we then also make monitor_ids required for now?
There was a problem hiding this comment.
@bkabrda that makes sense to me. I haven't actually updated any code here...should we just document that monitor_ids is required for now?
There was a problem hiding this comment.
Adjusted so monitor_ids is required
There was a problem hiding this comment.
@bkabrda this should be ready for another review when you can
|
can you remove this from the schema as well? |
|
Hmm, I'm honestly unsure about this. The Terraform deprecation policy [1] prevents us from just removing an attribute like this. OTOH this attribute never worked anyway, so I guess this should be fine... I think after all I'm 👍 for doing this change, but would like to hear more opinions. @nmuesch thoughts? [1] https://www.terraform.io/docs/extend/best-practices/deprecations.html#provider-attribute-removal |
|
since it's not in use, I think it's safe to just drop. |
|
One last question that I have here is if there's a specific timeframe on where Edit: to be more specific, if it's going to be supported soon(ish), maybe we should just document this but leave the field there. |
|
@bkabrda that option is currently not production ready and there is a good bit of design and dev work that needs to be done and hasn't been scheduled. We've already had a support case where someone was trying to use it and getting errors so we'd prefer to remove it. |
|
@unclebconnor ok, that's fair, let's remove the field for now. I was going through the terraform docs for removal of attribute [1]. The "right" way to do this seems to be to leave the field there, but add a [1] https://www.terraform.io/docs/extend/best-practices/deprecations.html#provider-attribute-removal |
|
@bkabrda Ok I've made that update. I'm pretty sure this is the format that is described in the docs but let me know if you have other feedback |
|
@bkabrda not sure of process for this repo but I'm assuming you want to do the final merge. Let me know if there's anything else you need from me here. |
|
LGTM now, merging. 👍 thanks for your patience :) |
removing
monitor_searchflag while unsupported