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

Get rid of Iris and use julienschmidt/httprouter instead #20

Merged
merged 5 commits into from
Nov 14, 2017
Merged

Conversation

joohoi
Copy link
Owner

@joohoi joohoi commented Nov 14, 2017

Iris can't keep the API stable, and while looking into it, found out a lot of drama behind the scenes too. This change will also rid us of hefty dependency stack brought in by iris.

Brought back the HTTP api tests as well!

Fixes #16
Fixes #11

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.9%) to 69.388% when pulling 4a22f32 on httprouter into 93871a7 on master.

Repository owner deleted a comment from coveralls Nov 14, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+23.5%) to 96.768% when pulling 1c401e7 on httprouter into 93871a7 on master.

@joohoi joohoi merged commit fd9ce46 into master Nov 14, 2017
http.Error(w, string(jsonError("body_missing")), http.StatusBadRequest)
return
}
json.NewDecoder(r.Body).Decode(&aTXT)
Copy link
Contributor

Choose a reason for hiding this comment

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

The error return value is not checked here, is this on purpose to allow empty POST bodies?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It indeed is the idea to allow empty POST bodies, as the only parameter for registration, "allowfrom" is optional.

You have good eyes though, as the body check shouldn't be there, even though it's really not completely empty in pretty much any real world situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is also a good point, however I was actually talking about the return value of Decode, which can be an error and is not checked in this case, which means that there is no indication that invalid JSON was sent. Of course, when you check it and the body is empty, it will return an error (probably EOF), so that should still be allowed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Let's continue the discussion on issue #23 so it doesn't get buried to the PR graveyard.

@joohoi joohoi deleted the httprouter branch January 22, 2018 08:22
jacobmyers-codeninja pushed a commit to jacobmyers-codeninja/acme-dns that referenced this pull request Sep 30, 2020
* Replace iris with httprouter

* Linter fixes

* Finalize iris removal

* Vendor dependencies for reproducable builds

* Api tests are back
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.

3 participants