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.

Sign up for GitHub

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

Jump to bottom

agent: consolidate handling of 405 Method Not Allowed #3405

Merged
merged 11 commits into from
Sep 26, 2017
Merged

agent: consolidate handling of 405 Method Not Allowed #3405

merged 11 commits into from
Sep 26, 2017

Conversation

magiconair
Copy link
Contributor

This patch consolidates the handling of the 405 Method Not Allowed status code and updates documentation and methods according to both each other and reality.

There is a new test which covers all endpoints and which replaces the spurious other tests for this status code.

I'm open to move the definition of which status codes are allowed somewhere else or to refactor the code to something more readable or better suited.

@magiconair
Copy link
Contributor Author

Hmm, two tests are still failing which I'll fix but I'd appreciate feedback on the approach nevertheless.

@magiconair magiconair changed the title Consolidate handling of 405 Method Not Allowed agent: consolidate handling of 405 Method Not Allowed Aug 19, 2017
@magiconair magiconair self-assigned this Aug 21, 2017
slackpad
slackpad reviewed Aug 22, 2017
View reviewed changes
Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

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

LGTM though I think with some of the slop that we had before this is considered a breaking change. We should probably hold this one until 1.0, unfortunately.

@slackpad
Copy link
Contributor

Thought about a helper so we don't have to stutter the methods into the error, but it seemed like overkill for 3 lines of code, so the approach looks good.

@magiconair
Copy link
Contributor Author

OK, then I'll just merge the doc change for now.

@slackpad slackpad added this to the Next milestone Sep 8, 2017
@slackpad slackpad added type/enhancement Proposed improvement or new feature theme/api Relating to the HTTP API interface labels Sep 8, 2017
@magiconair
Copy link
Contributor Author

Rebased to master

@magiconair magiconair force-pushed the fix-http-method-not-allowed branch 2 times, most recently from 0ca7c7e to d717120 Compare September 25, 2017 19:03
@magiconair @slackpad
agent: do not panic on metrics tests
5c2e377
@magiconair @slackpad
agent: consolidate http method not allowed checks
675ba81 
This patch uses the error handling of the http handlers to handle HTTP
method not allowed errors across all available endpoints. It also adds a
test for testing whether the endpoints respond with the correct status
code.
@magiconair @slackpad
agent: drop other tests for MethodNotAllowed
90b2b7c
@magiconair @slackpad
agent: align /agent/join with reality
ba48f38 
/agent/join uses PUT instead of GET as documented.
@magiconair @slackpad
agent: align /agent/check/{fail,warn,pass} with reality
cdd78fe 
/agent/check/{fail,warn,pass} uses PUT instead of GET as documented.
@magiconair @slackpad
fix some tests
4196683
@magiconair @slackpad
Drop more tests for method not allowed
e4f3815
@magiconair @slackpad
Align TestAgent_RegisterService_InvalidAddress with reality
923e359
@slackpad slackpad merged commit 1e46111 into master Sep 26, 2017
@slackpad slackpad deleted the fix-http-method-not-allowed branch September 26, 2017 06:11
@magnuswahlstrand
Copy link

Is there a way to disable this check in >1.0.0, like a flag? We have some legacy system that were run on 0.6.3, but I would like to use the consul docker with is using 1.0.1, thus breaking the legacy system.

Thanks!

@slackpad
Copy link
Contributor

Hi @kyeett unfortunately not - we wanted to break these and tighten up the handling as part of 1.0 so we would be in a better position to support Consul's interfaces for the long term.

@magnuswahlstrand
Copy link

magnuswahlstrand commented Dec 12, 2017 •
edited
Loading

A colleague helped me find the somewhat hidden consul:0.9.3 docker image :-) We will use that for now.
Thank you for your answer!

@andriisabitov andriisabitov mentioned this pull request Mar 7, 2018
Closed
andriisabitov added a commit to andriisabitov/ja-micro that referenced this pull request Mar 7, 2018
@andriisabitov
Change method to PUT for Consul health check update according to hash…
504c902 
…icorp/consul#3405
@TylerLubeck TylerLubeck mentioned this pull request Nov 16, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slackpad slackpad slackpad left review comments

@preetapan preetapan Awaiting requested review from preetapan

Assignees

@magiconair magiconair

Labels
theme/api Relating to the HTTP API interface type/enhancement Proposed improvement or new feature
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@magiconair @slackpad @magnuswahlstrand