[ISSUE-154] Add Watches

[RobsonSutton]

• Add Watches feature, raised under https://github.com/elastic/terraform-provider-elasticstack/issues/154
• ES Watcher API endpoints: https://www.elastic.co/guide/en/elasticsearch/reference/current/watcher-api.html

[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]

Hey again @tobio, Just wondering if you may be able to impart some wisdom on this PR? my head is sore from banging it against the desk currently, and I've been going in circles trying to figure out where I am messing things up...

ISSUE 1

I am struggling a bit with how to best handle the nested "active" field in the GET response... (sorry, probably super elementary!), seem to get an unmarshalling error between the bool type and model but can't quite pin down why this is being thrown currently as the different interactions seem to match up to me (must be missing something though!)...

Apologies if this is super simple, I've just been struggling to get my head around it for the past day or so to no avail... Any pointers or advice you have on how to tackle either would be greatly appreciated!

[tobio]

@RobsonSutton I've pushed up a commit which fixes the parsing problem. It looks like the code was expecting a collection of watches, but the GET watch API just returns the requested watch directly.

The tests are still failing because of some defaults being returned in the API response, but this should unblock you. LMK if you need any other help here. This is looking great!

[RobsonSutton]

@RobsonSutton I've pushed up a commit which fixes the parsing problem. It looks like the code was expecting a collection of watches, but the GET watch API just returns the requested watch directly.

The tests are still failing because of some defaults being returned in the API response, but this should unblock you. LMK if you need any other help here. This is looking great!

Awesome stuff! 🥳 Thanks for this, now that I have seen the issue I'm kicking myself for not noticing it last week.. was clearly too far down the rabbit hole to see..! Oh well, live and learn! thanks again for helping with this one, is much appreciated!

[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)

[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!