Skip to content

HTTP response format continued - #366

Merged
jehiah merged 14 commits into
nsqio:masterfrom
mreiferson:http_updates_366
Jun 17, 2014
Merged

HTTP response format continued#366
jehiah merged 14 commits into
nsqio:masterfrom
mreiferson:http_updates_366

Conversation

@mreiferson

Copy link
Copy Markdown
Member

@jehiah noticed that we didn't update nsqlookupd HTTP response formats in #330

took advantage of the opportunity to make the code a little more modular and reusable

@jehiah

jehiah commented Jun 4, 2014

Copy link
Copy Markdown
Member

LGTM. ready for this to land?

@mreiferson

Copy link
Copy Markdown
Member Author

not yet, gonna do the forwards/backwards compatibility crap between daemons

Comment thread nsqd/http.go

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

btw, I have to do this anonymous function junk to maintain compatibility with go1.0.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

much appreciated =).

inching closer to not needing that.

@mreiferson

Copy link
Copy Markdown
Member Author

I have a bit more work to do here but this is the last PR I'd like to get in for this next release

@mreiferson

Copy link
Copy Markdown
Member Author

this is ready for review, but I intend on doing a few more things:

  • manually give this a test run
  • get some tests written for old/new HTTP endpoints for nsqd and nsqlookupd

Comment thread util/api_response.go

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Technically we could put this in the actual content-type response "application/json; charset=utf-8; nsq-version=1.0"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would be worried about shitty HTTP clients that this might screw up...

I copied the GitHub API, FWIW.

@jehiah

jehiah commented Jun 17, 2014

Copy link
Copy Markdown
Member

Changes look pretty sane to me.

@mreiferson

Copy link
Copy Markdown
Member Author

OK, RFM @jehiah

I've deferred the nsqadmin tests to #323 - but I've tested it manually with and without nsqlookupd and with old and new versions.

jehiah added a commit that referenced this pull request Jun 17, 2014
@jehiah
jehiah merged commit a6912ff into nsqio:master Jun 17, 2014
@mreiferson
mreiferson deleted the http_updates_366 branch June 17, 2014 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants