Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/digitalocean: Add monitoring option to digitalocean droplets #13494

Closed
wants to merge 1 commit into from

Conversation

joshpurvis
Copy link

@joshpurvis joshpurvis commented Apr 10, 2017

This adds the monitoring option described in the DO API which was added to godo on this commit.

Caveats:
As far as I can tell, the current DO API doesn't allow you to turn off monitoring or read the monitoring state.

Due to this, I wasn't able to come up with a way to properly confirm that the option was applied on the other end after state is read.

Also, I assumed forcing new on a new option would be a bad idea in this case, so reverting the flag to false will have no effect on existing resources.

I'm new to go-- feel free to let me know if this needs some work.

@joshpurvis joshpurvis changed the title Add monitoring option to digitalocean provider provider/digitalocean: Add monitoring option to digitalocean droplets Apr 10, 2017
@lfarnell
Copy link

lfarnell commented Apr 13, 2017

I'm new to Terraform but after reviewing this PR I think you may have missed somethings. You should probably add a check for the state of monitoring on the read and add a check for update as well as the update much like how private_networking is done. Well done though otherwise!

I stand corrected. Currently it doesn't look like you can query the droplet to see if monitoring is enabled.

@skovorodkin
Copy link
Contributor

Closes #13376.

@stack72
Copy link
Contributor

stack72 commented Apr 15, 2017

Hi @joshpurvis

Thanks so much for the contribution here - in it's current format, I don't think I can merge it. There are a few reasons for this:

  1. If we cannot update the value, then we need to set the parameter ForceNew: true on the schema value. If we don't do this, then if a user disables monitoring in their config then they will believe something has changed when it hasn't

  2. We are not able to set the value back to state in the Read func as you pointed out. This will break import functionality and when a user sets the value, it will then force a new droplet

You pointed both of these things out of course, but I think we may need DigitalOcean to provide a fix for #2 and we can easily set #1 ourselves :)

What do you think?

Paul

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Apr 15, 2017
@joshpurvis
Copy link
Author

Yep I tend to agree. I've created a support ticket with Digitalocean.

@lfarnell
Copy link

Hi Luke,

Thanks for contacting us today. Sorry for the confusion with all of this. Monitoring management through the API is not fully implemented yet as you have seen. There isn't even a documentation section on it in the API docs at this stage. Our monitoring team is working on getting that fully implemented into the API soon as well as the documentation for it. Currently you are able to implement it but as you have seen this doesn't show up in the features. We do hope to see that available soon.

Let us know if you have any questions.

This is what DO responded with to me after I looked into it.

@paddycarver paddycarver added upstream and removed waiting-response An issue/pull request is waiting for a response from the community labels May 8, 2017
@lfarnell
Copy link

@joshpurvis It looks like DO has included the required field in the features block

"features": [
    "backups",
    "private_networking",
    "ipv6",
    "monitoring"
],

Might also have to move this PR to the new repository as well

@grubernaut
Copy link
Contributor

Hello @joshpurvis, and all!

Thank you for the contribution, we would love to be able to support this in the DigitalOcean provider!

However, with the migration of all of our providers to their new GitHub organizations (https://www.hashicorp.com/blog/upcoming-provider-changes-in-terraform-0-10), the DigitalOcean provider is now located at: https://github.com/terraform-providers/terraform-provider-digitalocean.

As such, I'm going to close this PR, as we cannot merge it in it's current state. If you wouldn't mind migrating this PR over to the new repository, I will happily review it for you (Just tag me as a requested reviewer).

Thanks again, and sorry for the inconvenience!

@ghost
Copy link

ghost commented Apr 8, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants