search

heroku / platform-api

Ruby HTTP client for the Heroku API
MIT License
213 stars 86 forks source link

Use heroics 0.0.21, load the config and the schema file at runtime to correctly map schema attributes used in metaprogramming #61

Closed djcp closed 7 years ago

djcp commented 7 years ago

I'm a dipshit and accidentally pushed this to master before submitting this PR. We could probably force push it off but I don't really care about a little churn in the history.

Otherwise, in terms of what this does, it uses the newly fixed heroics release and ensures the config and schema are loaded so that the config is the same during metaprogramming.

mathias commented 7 years ago

I'm not sure how I feel about adding the testing here. It makes me think we might want to set up a test user and record VCR cassettes (but they'd be hard to recreate when API changes -- and others couldn't contribute because they wouldn't have access to a test account.) Or we could Webmock everything and put some endpoint examples in a response fixtures dir based on the schema. The part I like least is actually talking to the Platform API.

Let me think about it a little bit.

djcp commented 7 years ago

@mathias

Yeah, I'm not a huge fan either, but I had limited goals with this testing: I just want to have some amount of confidence that the auto generated API works at a high level and that we haven't screwed up something deeply fundamental.

Webmock feels like it'd be too much work and potentially error prone. VCR might actually be a good fit here. The only thing you need (for now) to generate cassettes is an account with a single app. Maybe the best option here would be bring in VCR and then hook up travis, then we'll have much better tooling and confidence in this.

FWIW, the current platform-api gem is broken and it seems bad to hold up a release to figure out better testing (when none existed before).

tmaher commented 7 years ago

I just want to have some amount of confidence that the auto generated API works at a high level and that we haven't screwed up something deeply fundamental.

Something deeply fundamental is screwed up, though given the complexity of Heroics, I think that's utterly understandable. The one that has me particularly aggravated:

http://www.rubydoc.info/gems/platform-api/0.8.0/PlatformAPI%2FOrganizationMember%3Alist vs http://www.rubydoc.info/gems/platform-api/1.0.0/PlatformAPI%2FOrganizationMember%3Alist

The semantics for OrganizationMember#list changed from "List members of an organization" (one argument - the org name) to "List apps owned by a member" (two arguments - org name & user). There is currently no way I can find in the gem to "list the members of an organization".

Huge 👍 for VCR. I appreciate that it does make it slightly harder for new contributors to add unit tests. However, I think it'd be tremendously helpful to have Heroku provide a base set of cassettes/tests. Even if test coverage isn't 100%, a few basic tests could have prevented the buggy release.

/cc @jameswhite

tmaher commented 7 years ago

FWIW, the current platform-api gem is broken and it seems bad to hold up a release to figure out better testing (when none existed before).

To be brutally honest, testing did exist before, in the old heroku.rb gem - https://github.com/heroku/heroku.rb/tree/master/test . It seems equally bad to end compatibility for that given that testing wasn't carried forward into platform-api.

djcp commented 7 years ago

@tmaher I looked into changes for the PlatformAPI::OrganizationMember#list method and you're right that heroics is getting confused, it seems to have overwritten the original list method with this one in the the platform api reference. Obviously this is a bug.

mathias commented 7 years ago

This LGTM. Let's :shipit: and fix some more bugs.