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

remove unnecessary ruby 2.2 requirement #72

Merged
merged 5 commits into from Sep 15, 2016
Merged

remove unnecessary ruby 2.2 requirement #72

merged 5 commits into from Sep 15, 2016

Conversation

WattsInABox
Copy link
Contributor

I was unable to run all the tests b/c of this issue #71, but the two tests that did run passed. I'm letting travis take over.

See my comments on the PR on ruby-http-client for more info sendgrid/ruby-http-client#2

@thinkingserious
Copy link
Contributor

Hi @BillyWatson,

First of all, thanks for the pull request!

To merge it, we'll need a signed CLA: https://github.com/sendgrid/sendgrid-ruby/blob/master/CONTRIBUTING.md#cla

Apologies on the tests issue. The tl;dr; is that if you download prism: https://stoplight.io/prism/ and run the command: ./prism run --mock --list --spec https://api.stoplight.io/v1/versions/563a5309daad691100fb05af/export/stoplight.json a mock SendGrid server will spin up locally under port 4010 and the tests will run.

We are working on making this process automated (we already did so for Go https://github.com/sendgrid/sendgrid-go/blob/master/CHANGELOG.md#302---2016-07-07, both for local testing and within Travis so that the environment variable is no longer needed to access the hosted mock server). Please stay tuned for the update here in this library.

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: cla needed labels Jul 7, 2016
@WattsInABox
Copy link
Contributor Author

I'm working on the agreement.

My only issue with the prism stuff is that it should get documented ASAP in the contribution docs until it's changed to be automated. I'll be happy to submit another PR with changes to the README and such, if you'd like.

@thinkingserious
Copy link
Contributor

We have received the CLA, thanks!

If you want to add the Prism documentation in, that would be great! Otherwise, I'll open a ticket and get that prioritized.

@WattsInABox
Copy link
Contributor Author

I'll add the documentation in after I get the test environment set up. That way I've documented something that for sure works (at least for me), then we can go from there.

@WattsInABox
Copy link
Contributor Author

In other words, it might take me a couple hours to get it all going tomorrow morning (hopefully not).

@WattsInABox
Copy link
Contributor Author

Hi, I ran the server and it looks like a lot of the tests now pass. However, a lot of tests are still failing. But even when I run them on ruby 2.2, they continue to fail.

I noticed one of the failures was that an endpoint didn't exist and when I checked the stoplight.json, that endpoint definitely wasn't in there.

Is that the newest stoplight.json file?

@thinkingserious
Copy link
Contributor

@BillyWatson,

Yes, that link pulls the latest stoplight.json.

I just tested and no tests should be failing. Could you please tell me which tests are failing? Thanks!

@WattsInABox
Copy link
Contributor Author

okay after following the directions on the other PR, I'm now down to one failing test, which I should hopefully be able to figure out for ruby 2.1. The existing code works on 2.2, but I'll test whatever changes I make on both ruby versions to make sure I didn't break anything.

  1) Error:
TestAPI#test_contactdb_recipients_search_get:
URI::InvalidURIError: bad URI(is not URI?): http://localhost:4010/v3/contactdb/recipients/search?%7Bfield_name%7D=test_string&{field_name}=test_string
    /Users/williamrwatson/.rvm/rubies/ruby-2.1.8/lib/ruby/2.1.0/uri/common.rb:176:in `split'
    /Users/williamrwatson/.rvm/rubies/ruby-2.1.8/lib/ruby/2.1.0/uri/common.rb:211:in `parse'
    /Users/williamrwatson/.rvm/rubies/ruby-2.1.8/lib/ruby/2.1.0/uri/common.rb:747:in `parse'
    /Users/williamrwatson/.rvm/gems/ruby-2.1.8@sendgrid-ruby/bundler/gems/ruby-http-client-5a06da6e6d0a/lib/ruby_http_client.rb:131:in `build_url'
    /Users/williamrwatson/.rvm/gems/ruby-2.1.8@sendgrid-ruby/bundler/gems/ruby-http-client-5a06da6e6d0a/lib/ruby_http_client.rb:144:in `build_request'
    /Users/williamrwatson/.rvm/gems/ruby-2.1.8@sendgrid-ruby/bundler/gems/ruby-http-client-5a06da6e6d0a/lib/ruby_http_client.rb:221:in `method_missing'
    /Users/williamrwatson/development/sendgrid-ruby/test/sendgrid/test_sendgrid-ruby.rb:802:in `test_contactdb_recipients_search_get'

@thinkingserious
Copy link
Contributor

Looks like you fixed it with that last commit.

There was an error in our swagger spec that caused that, I've put in a request for a fix.

@thinkingserious
Copy link
Contributor

Note: merge this one first: sendgrid/ruby-http-client#2

@WattsInABox
Copy link
Contributor Author

@thinkingserious so I guess you're still waiting on that swagger fix? J/w cause I'm hoping to get the korrelate forks out of our project's Gemfile this week. Not the biggest deal in the world, though...

@thinkingserious
Copy link
Contributor

Thanks for checking in @BillyWatson,

I'm now waiting for this issue to bubble up in our backlog.

Priority can be bumped by other people add a thumbs up or commenting on the issue.

@addiedx44
Copy link

👍 Looks good to me.

@thinkingserious
Copy link
Contributor

Thanks for helping increase the priority @adamdunson!

@thinkingserious
Copy link
Contributor

Hello @BillyWatson,

This one finally made it to the top of my queue.

Do you mind fixing up the conflicts so I can merge?

Thanks!

@WattsInABox
Copy link
Contributor Author

Sure

@WattsInABox
Copy link
Contributor Author

@thinkingserious I've resolved everything and the tests are passing locally. Hopefully CI passes.

@WattsInABox
Copy link
Contributor Author

@thinkingserious yeah I think something is still wrong in the CI configuration

@thinkingserious thinkingserious merged commit 762d690 into sendgrid:master Sep 15, 2016
@thinkingserious
Copy link
Contributor

thinkingserious commented Sep 15, 2016

@BillyWatson Can you email us with your T-shirt size and mailing address to [email protected]? Thanks!

@thinkingserious
Copy link
Contributor

@BillyWatson,

Yes, it still needs the pull to come from someone belonging to the sendgrid org to work.

No worries though, I tested locally and everything was good. Thanks!

@thinkingserious
Copy link
Contributor

@BillyWatson,

Ah yes, there is some other issue going on with the tests. I think it's a Travis issue.

jamietanna pushed a commit to jamietanna/sendgrid-ruby that referenced this pull request Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants