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

Update heroku-go to cyberdelia/heroku-go/v3 #184

Closed
wants to merge 1 commit into from
Closed

Update heroku-go to cyberdelia/heroku-go/v3 #184

wants to merge 1 commit into from

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Aug 12, 2014

Hello Hashicorp –

TL;DR: switch Heroku Go library from bgentry/heroku-go to cyberdelia/heroku-go/v3

I wanted to take a stab at #116, but bgentry/heroku-go doesn't have support for the Heroku Build API.
I tried adding it in bgentry/heroku-go#10 but ran into issues, and Blake suggested that Tim's library (cyberdelia/heroku-go) might be a better way forward. Thankfully it includes the Build and App setup APIs I would need, but instead of 1 giant pull request that changes the library and adds the features of #116, I broke them in two. Not much sense in developing new things if the foundation they're built on gets rejected 😄

Porting over was pretty trivial; there are just a few minor differences:

  • heroku-go/v3 uses a heroku.Service type instead of a heroku.Client
  • ID fields are typically ID instead of Id
  • Some methods like AddonCreate take 2 params, an app name and a struct{config, plan}, instead of 3 params for name, config and plan.
  • some changing / removing of &'s

The latter of which may be "wrong". I'm still a Go novice, and may be using pointers when I should be using values, or values when I should be using pointers. The changes may simply be chalked up to differences in what the two libraries return/expect, and maybe not "wrong" at all ¯_(ツ)_/¯

Caveat(s):

The Heroku tests pass:

$ make test TEST=./builtin/providers/heroku
TF_ACC= go test ./builtin/providers/heroku  -timeout=10s
ok      github.com/hashicorp/terraform/builtin/providers/heroku 0.086s

The main test suite does not at the moment, neither on my branch or on master:

$ make test
TF_ACC= go test ./...  -timeout=10s
ok      github.com/hashicorp/terraform  0.060s
ok      github.com/hashicorp/terraform/builtin/bins/provider-aws    0.016s
ok      github.com/hashicorp/terraform/builtin/bins/provider-cloudflare 0.013s
?       github.com/hashicorp/terraform/builtin/bins/provider-consul [no test files]
ok      github.com/hashicorp/terraform/builtin/bins/provider-digitalocean   0.008s
[...]
2014/08/11 21:58:17 [ERROR] Error walking 'test_instance.foo': 1 error(s) occurred:

* error
2014/08/11 21:58:17 [INFO] Walking: test_instance.bar (Graph node: test_instance.bar)
[...]
2014/08/11 21:58:17 [INFO] Apply walk complete
--- FAIL: TestApply_vars (0.00 seconds)
panic: unsupported type: invalid [recovered]
    panic: unsupported type: invalid

goroutine 168 [running]:
runtime.panic(0x41c2840, 0xc208100460)
[...]
FAIL    github.com/hashicorp/terraform/command  0.121s
ok      github.com/hashicorp/terraform/config   0.052s
[...]
--- FAIL: TestContextValidate_requiredVar (0.00 seconds)
panic: unsupported type: invalid [recovered]
    panic: unsupported type: invalid
[...]
goroutine 17 [syscall]:
runtime.goexit()
    /Users/clint/Developer/Cellar/go/1.3/libexec/src/pkg/runtime/proc.c:1445
FAIL    github.com/hashicorp/terraform/terraform    0.062s
make: *** [test] Error 1

The full test output is here.

Let me know what you think!

Clint

@pearkes
Copy link
Contributor

pearkes commented Aug 19, 2014

Thanks @catsby! This is super helpful and I agree we should change the underlying libraries.

Unfortunately, the Heroku provider was just rewritten to use the new provider API, so I'll have to port these changes manually. Thanks!

@pearkes pearkes closed this Aug 19, 2014
@catsby
Copy link
Contributor Author

catsby commented Aug 19, 2014

oy, you all hate my would-be contributions here :)

Are you going to port it yourself then?
I can take a stab at rebasing this on the updated master if you're open to that, otherwise have at it 👍

@pearkes
Copy link
Contributor

pearkes commented Aug 19, 2014

Would be great to see a rebase, I just felt bad as it will hardly merge at all now!

@ghost ghost locked and limited conversation to collaborators May 5, 2020
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