[ISSUE-154] Add Watches

[RobsonSutton]

• Add Watches feature, raised under [Feature] Support for Watches #154
• ES Watcher API endpoints: https://www.elastic.co/guide/en/elasticsearch/reference/current/watcher-api.html
• Fix Logstash Pipelines Import example

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[RobsonSutton]

Currently blocked due to basic license on docker-elasticsearch instance:

Will have to look to see if it's possible to uplift to trial license at all

[tobio]

You should be able to start a trial within the test setup. The API is exposed in the Go client too

[RobsonSutton]

Hey @tobio! I'm keen to get this one finished before the next version is released of the provider (0.6.0 I'm assuming), did you know when you're planning on rolling out this next version? (just so I know how much time I have to play with!)

Note: Am hoping to get this sorted this week anyway but thought I'd check just in case it was to be rolled out imminently!

[tobio]

Likely 2-3 weeks I think. Would be great to have this one in, thanks @RobsonSutton

[RobsonSutton]

@tobio - Please can you review this one when you have a spare min please?

Think it should be nearly there now, just been trying to get the main stuff in place so that it can make it in before the version bump!👍

Things to note:

• Have tried to keep as close to how they are defined via the UI (in one large JSON string). Happy to rename body -> json_body in case you ever wanted to have both options of either defining it via json or via TF blocks though?
• I have forced the main 4 fields to be required which differs slightly from the API. This is to workaround the issue with these being set to defaults by the API (I played around for quite a while with defining defaults in all manner of ways to try to match this with the API, but figured this would probably just be the best approach in the end since user config will then always match up), do shout if you disagree with this approach though!
• I have not added throttle_period just yet, more due to time than anything else! (this sets throttle_period_in_millis in ES anyway it seems, but just allows for declaration in a more readable format: throttle_period = "10s" == throttle_period_in_millis = 10000)
• I have also just added error handling for metadata = {} for the time being, to prevent infinite plan noise due to omitempty. Tried without this being set but just ended up with the reverse error when metadata wasn't being set instead...
• Also not sure of the naming standard really for resources so just followed something like this approach: provider_api_package_resource - Looks a little weird with watcher_watch though... I doubt there will be any other resources added in this package either, so happy to rename if you'd prefer 👍

Any feedback you have for any parts that need to be changed or any potential improvement / better ways of doing things would be greatly appreciated as always!

(Also, forgive the no. of commits... 😆 my local machine couldn't handle running the tests and containers under make docker-testacc so was just having to use make install and running locally to complete most of the validation and testing)

[tobio]

👍 couple of comments inline. We could likely address them in a follow up PR aside from the resource level connection properties if you're short on time.

Have tried to keep as close to how they are defined via the UI (in one large JSON string). Happy to rename body -> json_body in case you ever wanted to have both options of either defining it via json or via TF blocks though?

I think we should add the individual properties to this resource eventually, it would help with avoid the issues with applying defaults to the field values. body should be fine though, it won't conflict with any of the field names.

I have forced the main 4 fields to be required which differs slightly from the API. This is to workaround the issue with these being set to defaults by the API (I played around for quite a while with defining defaults in all manner of ways to try to match this with the API, but figured this would probably just be the best approach in the end since user config will then always match up)

I think this is an ok tradeoff for now. As per above, longer term it feels like it would be nicer to have individual properties for these fields which I think would help solve the dramas here. I was trying to think through using DiffSuppressFunc for these, but it would likely be a lot of complexity for little benefit. Not a huge fan of this requirement, but it does seem like the right approach for now :)

I have not added throttle_period just yet, more due to time than anything else! (this sets throttle_period_in_millis in ES anyway it seems, but just allows for declaration in a more readable format: throttle_period = "10s" == throttle_period_in_millis = 10000)

I'm happy to approve this without it, but if you have time it would be great to support this just to avoid confusion.

Also not sure of the naming standard really for resources so just followed something like this approach: provider_api_package_resource - Looks a little weird with watcher_watch though... I doubt there will be any other resources added in this package either, so happy to rename if you'd prefer 👍

We already have elasticstack_elasticsearch_index (which would have had a similar stutter), so I think just elasticstack_elasticsearch_watch is fine.

[RobsonSutton]

@tobio

I think we should add the individual properties to this resource eventually, it would help with avoid the issues with applying defaults to the field values. body should be fine though, it won't conflict with any of the field names.

Will look at this one tomorrow 👍 I guess I was approaching this more from a consumer angle when focusing on the full json body approach, so that any watchers being moved to the provider (as opposed to being created from scratch) would need to be decoded, separated and re-encoded to each of the fields, whereas with body you can just point at a file containing the copy and paste of the UI watcher config. Isn't a massive job for consumers to instrument though so if it offers better control from the provider side then will see if I can quickly rework this one to support individual properties instead!

I have not added throttle_period just yet, more due to time than anything else! (this sets throttle_period_in_millis in ES anyway it seems, but just allows for declaration in a more readable format: throttle_period = "10s" == throttle_period_in_millis = 10000)

I'm happy to approve this without it, but if you have time it would be great to support this just to avoid confusion.

Will have another look at this one tomorrow too 👍 it is just be a human readable wrapper for throttle_period_in_millis though so I'm not sure if you wanted to support both fields in the provider, or just one of the two? e.g. if I were to set throttle_period to 5s, when the watcher is created in ES, it will actually set throttle_period_in_millis to the equivalent value (5000) within the watcher as opposed to it being it's own field.

[tobio]

it will actually set throttle_period_in_millis to the equivalent value (5000) within the watcher as opposed to it being it's own field

This sounds like setting throttle_period would also require specifying throttle_period_in_millis to the equivalent value to avoid an endless diff loop. Sorry, I think I misunderstood what you'd said earlier, it sounds like you're right to leave throttle_period out of the provider.

[RobsonSutton]

@tobio - Apologies for the delay, body should be split into separate fields now though, please let me know your thoughts! 👍

[tobio]

This is awesome, thanks for making the time for this contribution. IMO it's much nicer having those fields split out :)

[RobsonSutton]

No problem! 👍 Thanks as always for helping to advise with the Go side of things, is much appreciated!