-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Container services #49
Conversation
This commit initializes the needed services which are not mocked so tests can be executed in any environment with docker. Some default modifications (i.e: connection strings) were also made to current tests to accomodate them for this setup. A docker-compose.yml file is provided with all the necessary parameters for this services to be initialized. Future services can be added easily by extending this configuration file In addition a makefile has been introduced to simplify command execution
provided services Update test users to use circleci default services
I've tried to set-up circleci but I came across a Golang issue that was preventing CPU tests to PASS. Instead of ignoring those tests I've submitted an issue to Go (golang/go#11609) hoping that this gets fixed soon.
@marcosnils this is a great idea - thank you! i'm going to pull this down and do a little testing before merging. do you have any specific guidance on getting this up and running, or does the make file pretty much do everything that's needed? |
The requirements are:
The makefile takes care about the rest. |
@toddboom sometimes when you run the This has been a known issue? in docker / docker-compose and the community still haven't reached a consensus about how to handle this situations. When this happens running Further info can be read here: |
This allows to run tests in environments where DOCKER_HOST is used. This is extremely helpful when using boot2docker to run docker
I like the idea about using docker-compose for setting up different services (for a few of my projects I'm using it too for local dev and functional testing). Would it be an idea to add a However, since many services will be added to telegraf in the future (I assume), doesn't this add the risk that the list of containers to fetch and start is going to grow and grow (and the tests will be slower and slower)? Therefore, wouldn't it be better to implement mocks where possible? E.g. for testing databases something like https://github.com/DATA-DOG/go-sqlmock could be used. |
@brocaar I do agree that mocking / stubbing should be used when possible. I've encouraged the use of mocks in the README file (https://github.com/influxdb/telegraf/pull/49/files#diff-04c6e90faac2675aa89e2176d2eec7d8R178) to prevent the dependency growth problem you are mentioning. Regarding running the tests inside another container through compose, I've tried it before and IMHO I agree with you that even though it gives you better isolation and environment consistency, it takes away the flexibility of just going into the code locally and run The Makefile is just an alias to run all tests but a common use-case while coding is to run only the desired package / file tests. Also I usually find useful when working with services to connect from my local computer using a In my experience every time I've tried to automate by doing everything in containers (even running tests) I ended up exposing ports and running tests locally in favor of flexibility. |
What I mean with my comment about the |
@brocaar your idea is only viable when all services are mocked and you don't need docker containers as a dependency to run them. Meanwhile, I don't see any benefit of decoupling docker-compose from the makefile. |
@@ -12,7 +13,7 @@ func TestPostgresqlGeneratesMetrics(t *testing.T) { | |||
p := &Postgresql{ | |||
Servers: []*Server{ | |||
{ | |||
Address: "sslmode=disable", | |||
Address: fmt.Sprintf("host=%s user=postgres sslmode=disable", testutil.GetLocalHost()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the changes below it mean that the tests won't run as the current user any longer, which breaks the non-docker test setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evanphx AFAIK there's no non-docker
test setup in telegraf. The reason why this PR exists is because if you need to run telegraf tests as today you need to install almost all the services (which are not mocked) in your local machine and expect them to be set-up with the proper/default? (is there really a proper?) configuration.
This PR provides a docker based deterministic way to set-up all the services and run tests against them, so changing the default DB test user (BTW, I've used the default in the official postgres docker image) seems trivial as long as the tests run in a deterministic way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I had to change the user because the postgres docker image uses it to initialize the DB configuration. If I wanted to use some other user (like the default/session one) then I had to make a custom docker image which I didn't want to do.
amidst a bit of controversy, i'm going to merge this in. someone on our team will be taking this over officially on Monday, and i'm going to try to get this into a locally-testable state before then. thanks, @marcosnils! |
@toddboom if someone on your team needs help with this just ping me. BTW, I've noticed that some other services were added to telegraf, it might be a good idea to containerize those which can't be mocked. |
@marcosnils will do - i'll probably be running through a bunch of the open PRs tomorrow, so i'll ping you if i get stuck! |
This PR adds docker container services to telegraf so complex services can be tested in a local environment.
In addition I've also fixed some tests issues I found on the road while implementing this.
I've tried to add CI (circle) to the project but due to a bug in Go I was having some issues and decided to postpone it. You can read more about this in this commit: 6355228