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

Fix contradiction in terms with provider documentation (#10815) #10874

Merged
merged 2 commits into from
Jan 17, 2017

Conversation

OJFord
Copy link
Contributor

@OJFord OJFord commented Dec 21, 2016

This PR fixes a contradiction in the API access terminology used by Scaleway and Terraform; each previously using access_key for different parameters. Discussion in #10815.

I've also included a removal of 'ARM' from the documentation (e.g. 'ARM server') to reflect the fact that Scaleway now also provides x86 servers.

@OJFord
Copy link
Contributor Author

OJFord commented Dec 21, 2016

(Test failed 1/2 on Travis' timeout.)

@@ -53,7 +53,7 @@ var scalewayMutexKV = mutexkv.NewMutexKV()
func providerConfigure(d *schema.ResourceData) (interface{}, error) {
config := Config{
Copy link
Contributor

@nicolai86 nicolai86 Dec 21, 2016

Choose a reason for hiding this comment

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

please leave the access_key in for now, for backwards compatibility.

It could look like this:

apiKey := ""
if v, ok := d.Get("token").(string); ok {
  apiKey = v
} else {
  if v, ok := d.Get("access_key").(string); ok {
    apiKey = v
  }
}
…
Config{
  APIKey: apiKey,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, fixed.

@@ -10,17 +10,17 @@ import (
func Provider() terraform.ResourceProvider {
return &schema.Provider{
Schema: map[string]*schema.Schema{
"access_key": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep access_key here and mark it as Deprecated: true,, for backwards compatability.

e.g.

"access_key": &schema.Schema{
  Deprecated: true,
},
"token": &schema.Schema{
},
…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, fixed.

@nicolai86 nicolai86 removed their assignment Dec 21, 2016
@nicolai86 nicolai86 added the waiting-response An issue/pull request is waiting for a response from the community label Dec 21, 2016
@nicolai86 nicolai86 added provider/scaleway and removed provider/scaleway waiting-response An issue/pull request is waiting for a response from the community labels Jan 12, 2017
@nicolai86
Copy link
Contributor

hey @OJFord, thanks for making the requested adjustments. Sadly the tests are now broken:

# github.com/hashicorp/terraform/builtin/providers/scaleway
builtin/providers/scaleway/provider.go:17: cannot use true (type bool) as type string in field value
# github.com/hashicorp/terraform/builtin/providers/scaleway
builtin/providers/scaleway/provider.go:17: cannot use true (type bool) as type string in field value

would you mind adjusting the compilation errors?

@nicolai86 nicolai86 added the waiting-response An issue/pull request is waiting for a response from the community label Jan 12, 2017
The parameters previously termed by Terraform:

1. Organization
2. Access key

Are referred to, respectively, by Scaleway [0] as:

1. Access key
2. Token

which is a confusing contradiction for a user.

Since Scaleway terms (1) both 'access key' [0] and 'organization ID' [1],
@nicolai86 suggested keeping the latter as already used, but changing (2) for
'token'; removing the contradiction.

This commit thus changes the parameters to:

1. Organization
2. Token

Closes hashicorp#10815.

[0] - https://cloud.scaleway.com/#/credentials
[1] - https://www.scaleway.com/docs/retrieve-my-organization-id-throught-the-api
Scaleway now provides x86 servers [0] as well as ARM.

This commit removes 'ARM' from various references suggesting that might be the
only option.

[0] - https://blog.online.net/2016/03/08/c2-insanely-affordable-x64-servers/
@OJFord
Copy link
Contributor Author

OJFord commented Jan 13, 2017

@nicolai86 Sorted. Deprecated needs a message.

@nicolai86 nicolai86 removed the waiting-response An issue/pull request is waiting for a response from the community label Jan 13, 2017
@nicolai86
Copy link
Contributor

Thanks @OJFord for the changes. LGTM.

ping @stack72

@stack72
Copy link
Contributor

stack72 commented Jan 17, 2017

This LGTM :) Thanks for all the work on this @OJFord and @nicolai86

@stack72 stack72 merged commit ed2b959 into hashicorp:master Jan 17, 2017
nicolai86 added a commit that referenced this pull request Jan 17, 2017
properly fetch configuration from env - leftover from #10874
stack72 pushed a commit that referenced this pull request Jan 17, 2017
properly fetch configuration from env - leftover from #10874
@ghost
Copy link

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

3 participants