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

Swap bgentry/heroku-go for cyberdelia/heroku-go #239

Merged
merged 1 commit into from
Aug 27, 2014
Merged

Swap bgentry/heroku-go for cyberdelia/heroku-go #239

merged 1 commit into from
Aug 27, 2014

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Aug 27, 2014

Hello (again) Hashicorp –

This is an updated #184

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


Old description begin

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 ¯_(ツ)_/¯

Old description end


Specific callout to line 65 of heroku/provider_test.go:

-   config := rp.Meta().(*heroku.Client)
-   if config.Username != expectedEmail {
-       t.Fatalf("bad: %#v", config)
+   if heroku.DefaultTransport.Username != expectedEmail {
+       t.Fatalf("bad: %#v", heroku.DefaultTransport)
    }
-   if config.Password != expectedKey {
-       t.Fatalf("bad: %#v", config)
+   if heroku.DefaultTransport.Password != expectedKey {
+       t.Fatalf("bad: %#v", heroku.DefaultTransport)
    }

The config struct(?) no longer has a Username attribute, that's stored in the DefaultTransport struct now. I think this is correctly testing things, but I'm not 100% sure.

The Heroku specific tests pass, however the Heroku acceptance tests do not... but they aren't passing for me on master either...

@pearkes
Copy link
Contributor

pearkes commented Aug 27, 2014

Thanks a ton for making this work again, @catsby!

Agreed, this is a great change and it's good to be on the most active and up-to-date client lib. The failing tests seem to be unrelated as you noted, and due to an API change with Heroku. Region and application names seem to be required now, whereas they weren't before. I don't when this change was introduced, but it definitely broke the acceptance tests.

I'm going to fix those issues and verify the DefaultTransport change you noted.

Appreciate it, merging!

pearkes added a commit that referenced this pull request Aug 27, 2014
Swap bgentry/heroku-go for cyberdelia/heroku-go
@pearkes pearkes merged commit 9842577 into hashicorp:master Aug 27, 2014
@catsby
Copy link
Contributor Author

catsby commented Aug 28, 2014

Awesome, I saw your note on 02d18d6 as well, 👍

@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