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/ns1: Ensure provider checks for credentials #12920

Merged
merged 3 commits into from
Mar 23, 2017

Conversation

pashap
Copy link
Contributor

@pashap pashap commented Mar 21, 2017

Fixes: #12833

I somehow never added a check for the ns1 api key, this PR mimics other providers Config structures. Also, this PR ensures defaults for the ns1 api endpoint and whether to ignore ssl in the client(use terraform often for local development).

httpClient.Transport = tr
config := Config{
Key: d.Get("apikey").(string),
Endpoint: d.Get("endpoint").(string),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should follow the pattern of

if v, ok := d.GetOk("endpoint"); ok {
  config.Endpoint = v.(string)
}

when we are adding optional parameters to the config block

Sound ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, i wasnt thinking when moving away from GetOk. Making fix now.

@pashap
Copy link
Contributor Author

pashap commented Mar 21, 2017

I was wrong about what caused the error, it was a bug in the ns1-go client that wasnt properly handling 401s. Updating the ns1-go client via govendor and running acctests then will include in this PR

@stack72
Copy link
Contributor

stack72 commented Mar 21, 2017

Thanks for the work here @pashap :)

@kian
Copy link
Contributor

kian commented Mar 21, 2017

Seconded, thanks for this work @pashap!

@kian
Copy link
Contributor

kian commented Mar 22, 2017

Also noticed the NS1 provider seems to think resources are new/modified between sequential terraform apply calls. @pashap / @stack72 would you like me to fill out a new issue for that or leave the notes here?

@stack72
Copy link
Contributor

stack72 commented Mar 23, 2017

LGTM! The build failure is not related to this - it is a timing out test in the core

% make testacc TEST=./builtin/providers/ns1
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/23 11:54:41 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/ns1 -v  -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDataFeed_basic
--- PASS: TestAccDataFeed_basic (14.44s)
=== RUN   TestAccDataFeed_updated
--- PASS: TestAccDataFeed_updated (17.92s)
=== RUN   TestAccDataSource_basic
--- PASS: TestAccDataSource_basic (8.36s)
=== RUN   TestAccDataSource_updated
--- PASS: TestAccDataSource_updated (13.81s)
=== RUN   TestAccMonitoringJob_basic
--- PASS: TestAccMonitoringJob_basic (6.72s)
=== RUN   TestAccMonitoringJob_updated
--- PASS: TestAccMonitoringJob_updated (8.89s)
=== RUN   TestAccNotifyList_basic
--- PASS: TestAccNotifyList_basic (6.40s)
=== RUN   TestAccNotifyList_updated
--- PASS: TestAccNotifyList_updated (9.92s)
=== RUN   TestAccRecord_basic
--- PASS: TestAccRecord_basic (16.56s)
=== RUN   TestAccRecord_updated
--- PASS: TestAccRecord_updated (20.44s)
=== RUN   TestAccTeam_basic
--- PASS: TestAccTeam_basic (6.78s)
=== RUN   TestAccTeam_updated
--- PASS: TestAccTeam_updated (8.51s)
=== RUN   TestAccUser_basic
--- PASS: TestAccUser_basic (12.74s)
=== RUN   TestAccZone_basic
--- PASS: TestAccZone_basic (7.93s)
=== RUN   TestAccZone_updated
--- PASS: TestAccZone_updated (11.31s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/ns1	170.752s

@stack72 stack72 merged commit 93595d6 into hashicorp:master Mar 23, 2017
@pashap
Copy link
Contributor Author

pashap commented Mar 23, 2017

@kian would you mind making a new issue for that? thank you

mbfrahry pushed a commit that referenced this pull request Mar 28, 2017
* provider/ns1: Ensure provider checks for credentials

* provider/ns1: stick with GetOk for provider config vars

* provider/ns1: NS1 go client fixes for handling http errors
@ghost
Copy link

ghost commented Apr 15, 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 15, 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.

Terraform 0.9.0 panic using NS1 provider
3 participants