Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Allowed the setting of AccountEmail variable #1

Merged
merged 2 commits into from
Apr 7, 2016

Conversation

henrytk
Copy link
Contributor

@henrytk henrytk commented Apr 6, 2016

Why

Pingdom has single-user and multi-user accounts. When using a multi-user account you need to supply the account owner email in the Account-Email header when calling the API. We received this error message when using this library:

Error applying plan:

2 error(s) occurred:

* pingdom_check.ping_example: 403 Forbidden: Not permitted for account type.
* pingdom_check.example: 403 Forbidden: Not permitted for account type.

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

We want to be able to authenticate in multi-user Pingdom accounts. For this we need to pass an AccountEmail variable, which is the email address of the account holder.

What

Added logic to config.go to check if the Account Email variable is set and override the client object using the new constructor if appropriate.

How to review

To be reviewed in conjuction with the pull request on go-pingdom library

@@ -26,8 +27,15 @@ func (c *Config) Client() (*pingdom.Client, error) {
if v := os.Getenv("PINGDOM_API_KEY"); v != "" {
c.APIKey = v
}
if v := os.Getenv("PINGDOM_ACCOUNT_EMAIL"); v != "" {
c.APIKey = v
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be c.AccountEmail = v.

@russellcardullo
Copy link
Owner

Thanks for the PR. There are a few issues I found with this when testing:

  1. Setting account email via an environment variable incorrectly overrides API Key (see inline comment).
  2. When using a non-multi-user account Terraform will prompt the user for an account email:

Terraform file:

variable "pingdom_user" {}
variable "pingdom_password" {}
variable "pingdom_api_key" {}

provider "pingdom" {
    user = "${var.pingdom_user}"
    password = "${var.pingdom_password}"
    api_key = "${var.pingdom_api_key}"
}

resource "pingdom_check" "example" {
    type = "http"
    name = "my http check"
    host = "example.com"
    resolution = 5
}

Gives the following:

$ terraform plan
provider.pingdom.account_email
 Enter a value:

The second item looks similar to another issue I found: hashicorp/terraform#1456

I did test setting the default account email to "" instead of nil and that does prevent the prompt.

In pingdom/provider.go:

"account_email": &schema.Schema{
    Type:     schema.TypeString,
    Default:  "",
    Optional: true,
    },

Would you mind taking a look to see if those changes will work for your use case?

The AccountEmail variable can be used when generating requests
to populate the Account-Email header. This is required by the
Pingdom API to allow authentication in multi-user accounts.
When using a non-multi-user account Terraform will prompt
the user for an account email if no default value is provided.
Default value of an empty string as been added.
@henrytk
Copy link
Contributor Author

henrytk commented Apr 7, 2016

Thanks Russell. Regarding your first point, oops! All fixed and squashed into the commit. And your second point, good stuff, it does work with our use case. I have added this as a second commit to act as an audit of why that default value is in there.

I will be looking at the check type next, as our project (part of the UK Government Digital Service) only supports HTTPS and this seems to default to HTTP.

@russellcardullo
Copy link
Owner

Looks great! Merging this now.

@russellcardullo russellcardullo merged commit b06c3da into russellcardullo:master Apr 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants