pact-foundation / pact-go

Golang version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
http://pact.io
MIT License
831 stars 104 forks source link

Running Pact-Go Daemon in a Container #46

Closed zbintliff closed 5 years ago

zbintliff commented 6 years ago

Is there any way to limit which ports interactions can map to? i.e. main server is 6666 and then 20100-20200 are used for interactions?

Right now we just expose every port over 1024 in our container but would like a little more constraints than that.

zbintliff commented 6 years ago

To add, digging through the code I see that it picks randomly here. Would you be open to feature requests to limit that?

mefellows commented 6 years ago

I'd be open to a feature request to specify the port itself, and default to randomly picked as is it's current behaviour.

zbintliff commented 6 years ago

Awesome I'll whip up a PR. Can you add some color though? We have only done consumer side (daemon.MockService) but hope to do producer side well. As far as I can tell with the code, another listener is only stood up when dealing with consumers. Is this right?

zbintliff commented 6 years ago

Gave it an attempt in #47

zbintliff commented 6 years ago

Would love to get your feedback on #49 before I go through and fix all the tests.

mefellows commented 6 years ago

Thanks @zbintliff, I've been away last week but will look over in the next few days.

zbintliff commented 6 years ago

Hope you had a great time away, closing as this is resolved.

mefellows commented 6 years ago

I sure did. Thanks for you patience in getting this one in.

You're currently wrapped in a beta release v0.0.10. I'll see how this goes over the next few week or so and bump it to latest if all good. This will include updating documentation etc.

zbintliff commented 6 years ago

Also, I noticed here it says dsl/mock_service.go fails gofmt -s but when I write locally and diff there are no changes. Can't figure out what is wrong with it.

And a pre-release is awesome because we can build our jenkins pipeline around it for our clients still.

mefellows commented 6 years ago

Yeah, I don't get that either. Strange.

zbintliff commented 6 years ago

I just realized I did all of this so I could expose a set of known ports on the daemon/mockservice. Then i wrote it to bind to localhost :/

mefellows commented 6 years ago

Pull request welcome ðŸĪŠ

zbintliff commented 6 years ago

Have to apologize. Seems my PR didn't really solve any issue. The feature works, you can limit the ports the mock servers are spun up on. But it only works if daemon and mockservers are on localhost with the process running the dsl.

  1. The first issue is the fact that the DSL checks if port is open. If daemon is on a different host, no guarantees on the port being available. Also, that port will never be busy on the DSL process so will for each client started it will probably find the same port.
  2. If everything is on the same host/container, exposing ports aren't going to be needed.

I have some thoughts on how to fix #1 and that involves the daemon determining the port. The DSL just passes the whole allowed port string through, the daemon determines the port and returns it as part of the server again.

I just wanted to step back and run some things past you before I dig into that. Thinking of making the following three changes:

  1. If pact.Host is not 'localhost' bind to 0.0.0.0 for mock server
  2. DSL passes "range" of allowed ports. Daemon determines and provides port again. Similar to how it was done before my first PR.
mefellows commented 6 years ago

No worries.

You make some good points, I'm happy for both of those suggestions to be done - go for it.

That being said, we need to move away from the Daemon at some point anyway. I'm going to expedite the implementation options of removing it and will document it over in #31. Your thoughts would be much appreciated on that one.

springerigor commented 6 years ago

@mefellows Good job with https://github.com/pact-foundation/pact-go/tree/feat/daemonless! 👍 I managed to dockerize setup and run it without a daemon (consumer part so far).

mefellows commented 6 years ago

That's fantastic news, thanks @springerigor! Please, any further feedback you have is much appreciated.

gaelollivier commented 6 years ago

Hey @mefellows, thanks for the good work ! I'm currently implementing contract testing in a golang codebase and I stumbled around the same issue when trying to run pact-go inside Docker containers. I'm currently trying-out your feat/daemonless branch, and it seems to work like a charm 👍 Managed to run both my consumer + provider tests with it ! Looking forward to have it released, do you have any ETA for that ?

I'm not sure I can find the time to make code contributions, but here's some feedback I have so far with using pact-go with Docker:

Install Pact binaries

RUN cd /opt &&\ curl -LO https://github.com/pact-foundation/pact-ruby-standalone/releases/download/v1.43.0/pact-1.43.0-linux-x86_64.tar.gz &&\ tar xzf pact-*.tar.gz

ENV PATH="$PATH:/opt/pact/bin"

Copy sources

WORKDIR /go/src/path/to/your/sources COPY . .

Run all tests

CMD go test -v ./...

- Other issue I encountered is related to error printing. At first, my `Dockerfile` was not setting-up the `PATH` properly and my tests were failing to find the `pact-mock-service` program. However, the only output I had was something like this:

=== RUN TestConsumer exit status 1


It took me some time to figure-out the issue was that here https://github.com/pact-foundation/pact-go/blob/feat/daemonless/client/service_manager.go#L147, the error message was not actually printed. This is probably due to some buffering not being flushed before the program exits. So maybe not using `os.Exit(1)` or flushing stdout would help here.

Appart from these minor issues, I managed to run my tests without needing to run a daemon, which is awesome 👏 
mefellows commented 6 years ago

Thanks for this @gaelduplessix that's awesome. That os.exit is something to look at. Not sure we need it anymore.

We're probably looking at making it real in next month or two. Feedback on the API and install are the main things we're waiting on, aside from a good tidy up of the code (it's very POCy at the moment as we explored the API).

Will take this feedback onboard

mefellows commented 5 years ago

Closing this issue for three reasons:

  1. Removed use of os.Exit() in service manager (see c2dcd782)
  2. Daemon no longer exists in the latest release.
  3. @gaelduplessix 's docker container contains useful information for others to go on. I've considered created one for this project, but it's really not that difficult and most times I've done this I end up customising it to a specific set of needs anyway.