Fix #11 by separating the responsibility to store and run hooks #27
Conversation
Remove unused fields from hooks.DummyRunner Fix type name in DummyRunner description Fix some warnings during testing: ``` warn: Parser warning in file './apiary.apib': (warning code 10) message-body asset is expected to be a pre-formatted code block, separate it by a newline and indent every of its line by 8 spaces or 2 tabs on lines 3-4 warn: Parser warning in file './apiary.apib': (warning code 10) message-body asset is expected to be a pre-formatted code block, separate it by a newline and indent every of its line by 8 spaces or 2 tabs on lines 5-6 ```
|
@IngmarStein thanks for opening this! I have been meaning to get these fixed but haven't gotten around to it. Unfortunately the e2e tests seem to be failing. Since they haven't been run in a while I opened #28 to just test master to make sure any upstream dredd changes did not change anything (since the dredd version is not pinned in the build). Since that passed these changes have caused a breakage. I'll try to look at this sometime today to dig into why its failing. |
|
I think one of the issues is that the example app vendors a specific goodman version which of course doesn't include the commits above and therefore doesn't compile. The vendor.json file could be updated after merging. |
|
The cucumber tests now pass and the only remaining error is due to the old vendored goodman code in the example app. I hope you don't mind adding Go 1.8 to the test matrix. |
cmd/goodman/main.go
Outdated
| @@ -51,7 +51,7 @@ func main() { | |||
| } | |||
| }() | |||
| for retries := 5; retries > 0; retries-- { | |||
| runner, err := goodman.NewRunner("Hooks", hookServerInitalPort) | |||
| runner, err := goodman.NewRunner("HooksRunner", hookServerInitalPort) | |||
There was a problem hiding this comment.
😢 for magic strings. I wonder if its possible to look this up using reflection or some other mechanism?
There was a problem hiding this comment.
That would look like this:
rpcService := reflect.TypeOf((*hooks.HooksRunner)(nil)).Elem().Name()
for retries := 5; retries > 0; retries-- {
runner, err := goodman.NewRunner(rpcService, hookServerInitalPort)
and needs an import of github.com/snikch/goodman/hooks
Do you prefer that?
There was a problem hiding this comment.
I'm guessing this was the change that fixed the timeouts correct?
I was hoping to introduce a change that threw a more obvious error instead of experiencing timeouts. Now that I think about it I don't think that would have changed anything. I'm fine with things either way.
There was a problem hiding this comment.
Yep, that was the cause for the timeouts.
I've pushed the change to retrieve the struct name via reflection.
|
@IngmarStein +1 for the addition of Go 1.8. |
|
@IngmarStein can we remove the vendoring of the goodman packages from the example's vendor.json? I think that will fix the example run in the CI build. I want to keep that apart of the CI build to make sure its always working but didn't realize the drawbacks of vendoring it like I did. I did a proof of concept in #29 to see if it would still pass and I'm hoping if you make the same change it will work. Once the example works I think this is good to merge. |
|
@IngmarStein going to merge and tag a new release! Thanks for fixing this. |
This PR fixes the warnings printed for each method in Hooks that doesn't have a matching signature for RPC.