[WIP] added deploy key resource #9

sjauld · 2017-10-04T13:05:15Z

My first crack at adding a new resource - please let me know if anything else is needed.

make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?   	github.com/terraform-providers/terraform-provider-bitbucket	[no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccBitbucketDefaultReviewers_basic
--- PASS: TestAccBitbucketDefaultReviewers_basic (19.40s)
=== RUN   TestAccBitbucketDeployKey_basic
--- PASS: TestAccBitbucketDeployKey_basic (20.88s)
=== RUN   TestAccBitbucketHook_basic
--- PASS: TestAccBitbucketHook_basic (22.34s)
=== RUN   TestAccBitbucketRepository_basic
--- PASS: TestAccBitbucketRepository_basic (10.86s)
=== RUN   TestAccBitbucketRepository_camelcase
--- PASS: TestAccBitbucketRepository_camelcase (11.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-bitbucket/bitbucket	85.394s

pdecat

I'm not a maintainer but as I also submitted my first PR to add a group resource using the 1.0 API a few weeks ago, I was curious to see if we did things differently or not.

Anyway, here is my informal review.

pdecat · 2017-10-05T13:32:39Z

bitbucket/resource_deploy_key.go

+		Exists: resourceDeployKeyExists,
+
+		Schema: map[string]*schema.Schema{
+			"owner": &schema.Schema{


The API documentation refers to this as accountnamewhich is what I used in my own PR https://github.com/terraform-providers/terraform-provider-bitbucket/pull/6/files#diff-212cd6fa62f60904d8a923069edaa6c9R43

Not sure which is better.

Yeah I used owner to align with the existing resources in this repo. Atlassian's docs seem to interchangeably use accountname or username (maybe a 1.0 vs 2.0 difference), so I guess owner is as good as anything.

pdecat · 2017-10-05T13:33:03Z

bitbucket/provider_test.go

-		t.Fatal("BITBUCKET_PASSWORD must be set for acceptence tests")
+		t.Fatal("BITBUCKET_PASSWORD must be set for acceptance tests")
+	}
+	if v := os.Getenv("BITBUCKET_TEAM"); v == "" {


Not that my opinion matters at all, but I like the idea of introducing a BITBUCKET_TEAM environment variable.

pdecat · 2017-10-05T13:37:24Z

bitbucket/resource_deploy_key.go

+	deployKey_req, _ := client.Get(fmt.Sprintf("1.0/repositories/%s/%s/deploy-keys/%s",
+		d.Get("owner").(string),
+		d.Get("repository").(string),
+		url.PathEscape(d.Id()),


Why escape as the pk/id is an integer?

pdecat · 2017-10-05T13:39:55Z

bitbucket/resource_deploy_key.go

+		url.PathEscape(d.Id()),
+	))
+
+	log.Printf("ID: %s", url.PathEscape(d.Id()))


Shouldn't that be in debug level?

pdecat · 2017-10-05T13:40:52Z

bitbucket/resource_deploy_key.go

+	_, err = client.Put(fmt.Sprintf("1.0/repositories/%s/%s/deploy-keys/%s",
+		d.Get("owner").(string),
+		d.Get("repository").(string),
+		url.PathEscape(d.Id()),


Why escape as the pk/id is an integer?

sjauld · 2017-10-18T08:21:03Z

⏳

cwood · 2017-11-07T22:45:28Z

bitbucket/resource_deploy_key.go

+				ForceNew: true,
+				ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
+					value := v.(string)
+					pattern := "ssh-rsa AAAA[0-9A-Za-z+/]+[=]{0,3}( [^@]+@[^@]+)?"


We support more than just rsa. So this will break if you try to do a ecdsa key.

cwood · 2017-11-07T22:53:59Z

My only real concern is that the validation string wont really work since we support more than just RSA.

cwood · 2017-11-07T23:52:26Z

Sorry to have negative information. But essentially we are gonna be removing all of the 1.0 API's soon(can't give specifics - watch for a blog post). Iv talked to the devs who maintain and work on the apis for us and they havnt come up with any specifics. But it is similar to users/groups.

sjauld · 2017-11-09T22:21:39Z

Sorry, are you saying that Atlassian is removing support for adding deploy keys via the API? I'm a little confused...

pdecat · 2017-11-10T06:53:00Z

On the same stance here, confused, as the same arguments were invoked to decline #6.

cwood · 2017-11-10T07:12:14Z

Essentially bitbucket will not be implementing either of these apis for the 2.0 version. And I talked to our team who maintains these apis. And they are going to deprecate these apis. So I don’t want to add these features then have to remove them later with no real plan to migrate.

…

On Thu, Nov 9, 2017 at 10:53 PM Patrick Decat ***@***.***> wrote: On the same stance here, confused, as the same arguments were invoked to decline #6 <#6> . — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAO6NcWUJwMQ6z93h1s8IEt5eGgzD7YCks5s0_LMgaJpZM4PtlC8> .

sjauld · 2017-11-10T07:42:57Z

So to confirm, Atlassian will remove the existing feature enabling the programmatic management of deploy keys?

…

On Fri, 10 Nov 2017 at 6:12 pm, Colin Wood ***@***.***> wrote: Essentially bitbucket will not be implementing either of these apis for the 2.0 version. And I talked to our team who maintains these apis. And they are going to deprecate these apis. So I don’t want to add these features then have to remove them later with no real plan to migrate. On Thu, Nov 9, 2017 at 10:53 PM Patrick Decat ***@***.***> wrote: > On the same stance here, confused, as the same arguments were invoked to > decline #6 > < #6 > > . > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > < #9 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAO6NcWUJwMQ6z93h1s8IEt5eGgzD7YCks5s0_LMgaJpZM4PtlC8 > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH2eNwUbOIMq09vBMcdwoOhh-6a6aFjqks5s0_dOgaJpZM4PtlC8> .

cwood · 2017-11-10T18:30:16Z

Yea unfortunately. I am not happy about this either. Your best bet would to make a site/master issue and ask for a 2.0 version of the deployment keys. I asked our API team again and they confirmed it.

erikvanzijst · 2017-11-16T02:07:23Z

So to confirm, Atlassian will remove the existing feature enabling the programmatic management of deploy keys?

This is correct.

We don't yet have a date for when we turn off these APIs, but we will not be creating copies of these in 2.0. I've left a more complete motivation on the site/master ticket.

cwood · 2017-11-16T02:38:34Z

Thanks for clarifying Erik

…

On Wed, Nov 15, 2017 at 6:07 PM Erik van Zijst ***@***.***> wrote: So to confirm, Atlassian will remove the existing feature enabling the programmatic management of deploy keys? This is correct. We don't yet have a date for when we turn off these APIs, but we will not be creating copies of these in 2.0. I've left a more complete motivation on the site/master ticket <https://bitbucket.org/site/master/issues/15196/removal-of-10-api#comment-41240017> . — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAO6NWRSDRyYlqRyuhIen6PLiBh0fxx1ks5s25jbgaJpZM4PtlC8> .

Stuart Auld added 2 commits October 5, 2017 00:02

added deploy key resource

5033a02

added test for creating keys in a team

1a5d5dd

pdecat reviewed Oct 5, 2017

View reviewed changes

incorporate @pdecat feedbacl

b321be3

radeksimko added the new-resource label Oct 9, 2017

cwood reviewed Nov 7, 2017

View reviewed changes

cwood closed this Nov 7, 2017

danstutzman mentioned this pull request Dec 5, 2017

Add bitbucket_repository_deploy_key resource #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] added deploy key resource #9

[WIP] added deploy key resource #9

sjauld commented Oct 4, 2017

pdecat left a comment

pdecat Oct 5, 2017

sjauld Oct 5, 2017

pdecat Oct 5, 2017

pdecat Oct 5, 2017

pdecat Oct 5, 2017

pdecat Oct 5, 2017

sjauld commented Oct 18, 2017

cwood Nov 7, 2017

cwood commented Nov 7, 2017

cwood commented Nov 7, 2017

sjauld commented Nov 9, 2017

pdecat commented Nov 10, 2017

cwood commented Nov 10, 2017 via email

sjauld commented Nov 10, 2017 via email

cwood commented Nov 10, 2017

erikvanzijst commented Nov 16, 2017

cwood commented Nov 16, 2017 via email

[WIP] added deploy key resource #9

[WIP] added deploy key resource #9

Conversation

sjauld commented Oct 4, 2017

pdecat left a comment

Choose a reason for hiding this comment

pdecat Oct 5, 2017

Choose a reason for hiding this comment

sjauld Oct 5, 2017

Choose a reason for hiding this comment

pdecat Oct 5, 2017

Choose a reason for hiding this comment

pdecat Oct 5, 2017

Choose a reason for hiding this comment

pdecat Oct 5, 2017

Choose a reason for hiding this comment

pdecat Oct 5, 2017

Choose a reason for hiding this comment

sjauld commented Oct 18, 2017

cwood Nov 7, 2017

Choose a reason for hiding this comment

cwood commented Nov 7, 2017

cwood commented Nov 7, 2017

sjauld commented Nov 9, 2017

pdecat commented Nov 10, 2017

cwood commented Nov 10, 2017 via email

sjauld commented Nov 10, 2017 via email

cwood commented Nov 10, 2017

erikvanzijst commented Nov 16, 2017

cwood commented Nov 16, 2017 via email