Dumb implementation for support --port and --host for hooks server #712

ddelnano · 2017-02-08T04:55:22Z

🚀 Why this change?

To allow compliant hooks servers to listen on different ports and ip addresses

📝 Related issues and Pull Requests

apiaryio/dredd-hooks-template#11
apiaryio/dredd-hooks-template#17

✅ What didn't I forget?

Still very much WIP since I believe there will need to be a discussion on what flags the hooks servers should look for (php already uses --port and --host but this can be up for discussion), how to stage these changes amongst the projects, etc. I just wanted to get the simplest thing done so that I could run the dredd-hooks-php cucumber tests with this new test to validate it works properly.

honzajavorek

Thanks very much! Especially for discovering the discrepancy in Dredd's universe. I like the change, but several things need to be resolved first:

There should be the discussion about how the parameters should look and work, as you mention, but honestly, --port and --host look quite production-ready to me 😄 The discussion should happen in Add test for configurable port dredd-hooks-template#17.
We need to find out how other hook libs will behave if they're given --port and --host before releasing this or similar change. People can have older installations while upgrading Dredd itself. I'm not sure whether other hook handlers will at least ignore the unknown arguments 🤔 (see Tests do not validate hooks can listen on different ports dredd-hooks-template#11 (comment))
I'd like Drop support for Node 0.10 and 0.12 #716 to be merged first so we get rid of the failing builds.

honzajavorek · 2017-02-15T09:49:11Z

src/hooks-worker-client.coffee

@@ -163,9 +163,10 @@ class HooksWorkerClient
        callback()

  spawnHandler: (callback) ->
-    pathGlobs = [].concat @runner.hooks?.configuration?.options?.hookfiles
+    pathGlobs = [].concat ["--port", "#{@handlerPort}", "--host", "#{@handlerHost}"], @runner.hooks?.configuration?.options?.hookfiles
+    console.info pathGlobs


Is this a debugging leftover? Maybe it was meant as logger.info?

Yep, removed it.

honzajavorek · 2017-02-15T09:49:45Z

src/hooks-worker-client.coffee

@@ -163,9 +163,10 @@ class HooksWorkerClient
        callback()

  spawnHandler: (callback) ->
-    pathGlobs = [].concat @runner.hooks?.configuration?.options?.hookfiles
+    pathGlobs = [].concat ["--port", "#{@handlerPort}", "--host", "#{@handlerHost}"], @runner.hooks?.configuration?.options?.hookfiles


In this case, I'd probably call the variable args instead of pathGlobs (with subsequent changes in the code below).

Also, lately I tend to prefer to have all Dredd options in the GNU-style --option=value format, consistently. In the case of Dredd itself, both formats should work, I just wanted to choose one way to go and stick with it.

Updated both of these.

ddelnano · 2017-02-21T01:36:06Z

Todo

Find out impact for existing hook servers
- php: compliant on most recent release (1.1.5), prior to that would have been unaffected.
- go: would break existing hook servers.
- ruby: should be ok ~~(need to verify with test since I'm not confident with my ruby knowledge :) )~~ Update: verified it is good.
- python: should be ok -- passes argv to glob which should ignore the new flags (need to verify but I'm more confident than the ruby one)
- perl: @ungrim97 can you help out? Literally know nothing about perl. Also you might want to checkout this if you haven't seen it already.
Finish discussion on if this is what we want. I like the idea of Dredd being able to discover if hooks support these flags at runtime but this is a much bigger change.

ungrim97 · 2017-02-21T07:59:43Z

Will require an update to the perl hook script to support the port/host arguments. Passing them in to the current version shouldn't break anything. (it will look for hookfiles files called --port=1234 and --host=127.0.0.1 but obviously won't find one and will skip them)

Will make an update to accept the new params

ungrim97 · 2017-02-21T09:16:50Z

ungrim97/Dredd-Hooks#7 should cover whats needed to make the Perl version work for this

w-vi · 2017-02-21T11:30:45Z

I'll check Python hooks and see if I can implement this feature quickly.

ddelnano · 2017-02-21T17:42:18Z

@honzajavorek @w-vi @ungrim97 so looks like all existing hooks servers will not be affected by this change except the Go ones. How do we want to handle that? I can get those updated soon but wanted to understand better what the plan was.

w-vi · 2017-02-21T21:11:28Z

Python hooks won't do anything bad, will skip the args so no issue here.

ddelnano · 2017-02-21T22:58:10Z

But what I mean is do we want to continue down this path? It's going to cause existing go hook servers problems. All other hooks seem fine but still.

honzajavorek · 2017-02-27T10:51:01Z

I'm sorry for not being prompt in answering (got sick). @ddelnano thanks very much for checking all the hooks! This is amazing work.

I see three options how to proceed:

Let the older Go hooks installations crash. I don't like it very much, but I can't decide, since I have very little insight into how many people use the Go hooks, how prompt they are in upgrading to newer versions, etc. Probably up to @ddelnano and @snikch to consider.
The features event mentioned by @w-vi in Add test for configurable port dredd-hooks-template#17 (comment) I like the idea, but implementing it would involve a lot of coding - both on hooks and Dredd sides. I'd prefer to have this as a future improvement instead of an instant solution. (I turned it into a separate issue: Hook handlers could provide Dredd with a list of supported features #723)
Hardcoded check. Is there a way we could hard code a simple check for Go hooks version? It's the least visually appealing solution, but doesn't involve much coding, doesn't make anything crash, and gets the job done.

What do you think?

ddelnano · 2017-02-27T19:50:05Z

@honzajavorek I definitely want to avoid option 1 :). And since the features event seems like alot more work than what we need for now some type of hard coded check will be necessary. I am going to brainstorm some ideas the rest of today and post back some potential ideas.

ddelnano · 2017-03-20T19:03:27Z

@honzajavorek sorry for the long delay, had an unexpected hospital trip but I'm doing much better.

Another way we could check the hooks server that I thought of that would be very dumb but get the job done. We could have dredd try and run the hooks server with the --host and --port arguments and if it fails try it without the --host and --port flags. The drawback is that unrelated failures will probably fail twice. For the hardcoded version check on the Go hooks how would we accomplish that? There is no way to check the version using the go hook binary.

honzajavorek · 2017-03-21T13:24:37Z

@ddelnano Whoa! 🚑 Whatever it was, I wish you to get better quickly!

The fact there is no way we can hardcode a check for go hooks version is unfortunate. I'll think about the trial & error approach. At first sight, I don't like it very much, but at the same time, it can be the only way we're left with to save the situation 🙂

ddelnano · 2017-03-21T13:29:07Z

@honzajavorek thanks, things are back to normal now :).

One thing I didn't think of for the hardcoded check. In the next version of the go hooks we could add a command to output the version and then if it doesn't support the version command we can assume that it won't support the --host and --port.

honzajavorek · 2017-03-24T08:50:00Z

@ddelnano What is the current behavior of go hooks if they're provided with --version? Do they start and wait for connection, do they crash, ...?

ddelnano · 2017-03-24T09:34:54Z

It's going to crash...

honzajavorek · 2017-03-24T09:58:02Z

Then I think we're okay with what you proposed in #712 (comment). Dredd could try to run the hook handler with options and if it crashes, it tries to run it without the options. If it doesn't crash, Dredd respects the test results.

ddelnano · 2017-03-24T10:17:50Z

Ok. Yea obviously not ideal but there aren't many options unfortunately. This weekend I will have some time I'll give that a shot. And work on getting the go hooks updated to use the new flags.

honzajavorek · 2017-11-29T16:14:15Z

I'm very sorry we were not able to get back to this. I need to close this as we're going to work on #705 first, which requires having no pending Pull Requests. We'll get back to this proposal as soon as we're addressing #917. Finishing this PR is listed on #917 as one of the necessary steps, so it shouldn't get forgotten. Thank you for the contribution! I hope we'll have the chance to tidy up the template soon.

ddelnano mentioned this pull request Feb 8, 2017

Add test for configurable port apiaryio/dredd-hooks-template#17

Open

3 tasks

honzajavorek reviewed Feb 15, 2017

View reviewed changes

ddelnano force-pushed the support-hook-handler-port-and-host branch from c293998 to 47369b8 Compare February 21, 2017 11:00

ddelnano added 5 commits February 21, 2017 11:47

feat: add host and port flags when spawning hooks

4d387f6

fix: Remove debug statement

8c310ac

fix: Remove variable to better reflect usage

7a1fe8d

fix: Use GNU style options

6cd6176

fix: Update assertions to reflect GNU style options

bf2e480

ddelnano force-pushed the support-hook-handler-port-and-host branch from 47369b8 to bf2e480 Compare February 21, 2017 16:49

ddelnano closed this Feb 27, 2017

ddelnano reopened this Feb 27, 2017

honzajavorek added the in progress label Jul 24, 2017

hobofan mentioned this pull request Aug 7, 2017

Hook socket message (documentation) inconsistencies #834

Closed

honzajavorek mentioned this pull request Nov 21, 2017

Resolve all pending problems with the current Dredd hook template #917

Open

honzajavorek closed this Nov 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dumb implementation for support --port and --host for hooks server #712

Dumb implementation for support --port and --host for hooks server #712

ddelnano commented Feb 8, 2017 •

edited

Loading

honzajavorek left a comment •

edited

Loading

honzajavorek Feb 15, 2017

ddelnano Feb 21, 2017

honzajavorek Feb 15, 2017

honzajavorek Feb 15, 2017 •

edited

Loading

ddelnano Feb 21, 2017

ddelnano commented Feb 21, 2017 •

edited

Loading

ungrim97 commented Feb 21, 2017 •

edited

Loading

ungrim97 commented Feb 21, 2017

w-vi commented Feb 21, 2017

ddelnano commented Feb 21, 2017

w-vi commented Feb 21, 2017

ddelnano commented Feb 21, 2017

honzajavorek commented Feb 27, 2017 •

edited

Loading

ddelnano commented Feb 27, 2017

ddelnano commented Mar 20, 2017

honzajavorek commented Mar 21, 2017

ddelnano commented Mar 21, 2017

honzajavorek commented Mar 24, 2017

ddelnano commented Mar 24, 2017

honzajavorek commented Mar 24, 2017 •

edited

Loading

ddelnano commented Mar 24, 2017

honzajavorek commented Nov 29, 2017

Dumb implementation for support --port and --host for hooks server #712

Dumb implementation for support --port and --host for hooks server #712

Conversation

ddelnano commented Feb 8, 2017 • edited Loading

🚀 Why this change?

📝 Related issues and Pull Requests

✅ What didn't I forget?

honzajavorek left a comment • edited Loading

Choose a reason for hiding this comment

honzajavorek Feb 15, 2017

Choose a reason for hiding this comment

ddelnano Feb 21, 2017

Choose a reason for hiding this comment

honzajavorek Feb 15, 2017

Choose a reason for hiding this comment

honzajavorek Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

ddelnano Feb 21, 2017

Choose a reason for hiding this comment

ddelnano commented Feb 21, 2017 • edited Loading

Todo

ungrim97 commented Feb 21, 2017 • edited Loading

ungrim97 commented Feb 21, 2017

w-vi commented Feb 21, 2017

ddelnano commented Feb 21, 2017

w-vi commented Feb 21, 2017

ddelnano commented Feb 21, 2017

honzajavorek commented Feb 27, 2017 • edited Loading

ddelnano commented Feb 27, 2017

ddelnano commented Mar 20, 2017

honzajavorek commented Mar 21, 2017

ddelnano commented Mar 21, 2017

honzajavorek commented Mar 24, 2017

ddelnano commented Mar 24, 2017

honzajavorek commented Mar 24, 2017 • edited Loading

ddelnano commented Mar 24, 2017

honzajavorek commented Nov 29, 2017

ddelnano commented Feb 8, 2017 •

edited

Loading

honzajavorek left a comment •

edited

Loading

honzajavorek Feb 15, 2017 •

edited

Loading

ddelnano commented Feb 21, 2017 •

edited

Loading

ungrim97 commented Feb 21, 2017 •

edited

Loading

honzajavorek commented Feb 27, 2017 •

edited

Loading

honzajavorek commented Mar 24, 2017 •

edited

Loading