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

Add REPORT as valid HTTP verb #390

Merged
merged 7 commits into from
Jan 22, 2017
Merged

Add REPORT as valid HTTP verb #390

merged 7 commits into from
Jan 22, 2017

Conversation

ixti
Copy link
Member

@ixti ixti commented Jan 22, 2017

Resolves #387.

@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

#387 issue and this PR actually brings us to the need of rethinking how we validate HTTP verbs. REPORT used in RFC 6352 extends RFC 3253. And taking a glance look at last one it's obvious that our current way will fail on VERSION-CONTROL method.

As of bundler 1.14.0 installing rainbow gem fails due sto bug in
rubygems that was fixed. For details see rainbow issue discussion:
ku1ik/rainbow#48 (comment)
@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

@tarcieri you might wanna about some of the changes made with this pr ;D

@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

@tarcieri I tend to merge this PR as failures on jruby now are pretty specific to travis and bundler. Somhow it simply fails to run on jruby on travis - but runs locally.

- 2.0.0
- 2.1
- 2.2
- 2.3.0
- 2.3.1
- 2.3.2
- 2.3.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point in testing every version of 2.3 under the sun?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I have no answer on this question.

- 2.4.0
- ruby-head
- jruby-9.1.6.0
- jruby-9.1.7.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you put jruby at the top it will start running first... which is a good thing as it takes the longest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

- 2.4.0
- ruby-head
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to be a fan of putting "ruby-head" in but nowadays I think it's just pointless

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I tend to agree. I mean I think it would make sense if build would be triggered at least nightly if not on each ruby-head commit.

@@ -3,6 +3,7 @@
.config
.rvmrc
.yardoc
.ruby-version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? I can see an argument for not having one, but why put it in .gitignore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding .ruby-version to .gitignore so that nobody will commit it accidentally... :D I mean I'm fine when .ruby-version is committed in repo, when only that ruby version is supported (e.g. for some ruby app), but overwise it's as good as adding ruby 2.4 line into Gemfile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a .ruby-version to most projects, with the latest version being supported. It automatically configures multiple development tools and is quite handy that way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow it was giving me more of a headache than helping. But I will revert that commit.

@tarcieri
Copy link
Member

Looks good... well aside from the JRuby failure

abotalov added a commit to abotalov/form_data.rb that referenced this pull request Jan 22, 2017
- define FLAKY_ENV const instead of flaky_env? helper
- use `:flaky` tag on examples to mark that it should be excluded on
  `FLAKY_ENV`, instead of `skip "..." if FLAKY_ENV`
- do not run tests coverage analysis on FLAKY_ENV
@ixti ixti force-pushed the enhancement/report-http-verb branch from 750b530 to 5f0935e Compare January 22, 2017 17:54
@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

@tarcieri master is also failing on jruby ;)) btw

@tarcieri
Copy link
Member

@ixti I'm asking on the Bundler Slack about it.

Maybe we should pin to a known-working Bundler.

@tarcieri
Copy link
Member

This appears to be the issue: rubygems/bundler#5340

@tarcieri
Copy link
Member

This will be fixed in Bundler 1.14.2

@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

Yeah. Thanks for digging into that. I have locked down bundler on travis for now to the one that worked last time for us ;))

@ixti ixti force-pushed the enhancement/report-http-verb branch 4 times, most recently from d163fa1 to a5a001f Compare January 22, 2017 20:05
@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

Okay. I have no clue how to lock bundler...

@ixti ixti force-pushed the enhancement/report-http-verb branch from a5a001f to acac075 Compare January 22, 2017 20:19
@tarcieri
Copy link
Member

Let me take a crack at just fixing that issue...

@ixti ixti force-pushed the enhancement/report-http-verb branch 2 times, most recently from e33f338 to 79797ad Compare January 22, 2017 20:38
@ixti ixti force-pushed the enhancement/report-http-verb branch from 79797ad to 84dcb1f Compare January 22, 2017 20:47
@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

After some time and couple of rounds when I thought I finally got insane - I have managed travis to run with bundler / rubygems known to work :D

@tarcieri
Copy link
Member

Looks good!

@ixti ixti merged commit 3b4a57c into master Jan 22, 2017
@ixti ixti deleted the enhancement/report-http-verb branch January 22, 2017 21:04
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Feb 5, 2017
Upstream changes (from CHANGES.md):

## 2.2.0 (2017-02-03)

* [#375](httprb/http#375)
  Add support for automatic Gzip/Inflate
  ([@Bonias])

* [#390](httprb/http#390)
  Add REPORT to the list of valid HTTP verbs
  ([@ixti])


## 2.1.0 (2016-11-08)

* [#370](httprb/http#370)
  Add Headers#include?
  ([@ixti])

* [#364](httprb/http#364)
  Add HTTP::Response#connection
  ([@janko-m])

* [#362](httprb/http#362)
  connect_ssl uses connect_timeout (Closes #359)
  ([@TiagoCardoso1983])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants