-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fixed the services
function so that Drone can launch containers on custom images.
#80
Conversation
this would be very useful! I want to run a postgres 9.3 container, and a few other services so it'd be great to be able to specify them by image name |
@sjhewitt Thank you very much for your comment! |
👍 just found myself looking for this. Although taking a look at this PR I think I'd prefer the service config to be more expressive, maybe even a full Use case for me is I want to start up a mongodb service, but with an additional flag (--replSet) that the default bradrydzewski/mongodb image does not have. |
I'd like to propose some modifications to this PR Instead of hard-coding the port(s) in the yaml file we could query the ports using the Docker api:
This means we wouldn't need to hard-code ports in the Go code either: The yaml could look like this: services:
- mongo # standard mongo image
- mongo:2.6 # standard mongo image, version 2.6
- foo/mongo # custom mongo image
- foo/mongo:2.6 # custom mongo image, version 2.6 @rgarcia the startup commands for mongo are actually hard-coded into the Dockerfile. If you wanted to specify additional startup parameters (like --replSet) you would need to create your own mongo Docker image. You could then specify it in the yaml like this:
|
@bradrydzewski even for images with entrypoint set you can customize the entrypoint arguments:
It'd be nice to have a service format flexible enough to do this. |
cool. didn't know that was possible! we could do this:
and just parse any trailing items and send to the Docker API as command args. Let's add this as a separate PR once the other changes are merged. thanks for the feedback! |
@bradrydzewski Thanks for your reviewing. I checked the role of As a conclusion, I think this This But normally, exposed ports have been defined already in the Dockerfile's EXPOSE instruction like this and we don't need to set these ports when calling Docker's I'll show you a few examples.
$ root@dev:~# docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
960bcf36d862 bradrydzewski/mongodb:2.4 usr/bin/mongod --nop 8 minutes ago Up 8 minutes 127.0.0.1:49156->27017/tcp grave_babbage Even if I changed this $ root@dev:~# docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
960bcf36d862 bradrydzewski/mongodb:2.4 usr/bin/mongod --nop 8 minutes ago Up 8 minutes 127.0.0.1:49156->27017/tcp grave_babbage When I changed this $ root@dev:~# docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
597d9b601686 bradrydzewski/mongodb:2.4 usr/bin/mongod --nop 2 seconds ago Up 1 seconds 127.0.0.1:49158->27011/tcp, 27017/tcp tender_nobel From the above, we don't need to specify the exposed ports when calling Docker's What do you think? |
we use the ports here: databases are exposed to the docker container using a local IP address (not localhost). The above code will proxy the databases (or services) over localhost to the external container and IP address. This is important because it lets your write your unit tests and database tests using localhost, even though the database isn't running on localhost :) |
Thanks for your reply. I understood. I'll try to fix later. |
@yosssi I also found your pull request very useful: I merged on my drone fork (https://github.com/xpepper/drone), deployed on my drone installation and it works! This was particularly useful for running a build on a project which needed the elasticsearch service and since the default service seems to be broken (see #89) I used the official build from dockerfile (https://index.docker.io/u/dockerfile/elasticsearch/) which (thanks to your patch which enables custom services) works great.
I think this pull request is worth merging. |
@xpepper I also built drone based on your current master to try out my postgres 9.3 image (see drone/images#9) and it works quite nice. However I also think it would be even cleaner if the port would be extracted from the database container. After all we're not defining any ports for the default services neither. So @yosssi if you find the time to fix that it would be super-awesome! |
Great @yosssi , looking forward to it! |
Given the popularity of this request I went ahead and implemented the change: Here is the specific code if anyone is interested: You can use the following yaml syntax: services:
- elasticsearch # this gets converted to bradrydzewski/elasticsearch:0.9
- johnsmith/redis # this gets converted to johnsmith/redis:latest
- johnsmith/postgres:9.3 The mock unit tests are all passing, however, I haven't tested this against a real Docker client yet. I'll get to it some time this week. With some help testing we can expedite this. |
@bradrydzewski awesome, thanks a lot! I just built and deployed your branch and tested it with this
Works perfectly! Not that I built the postgres 9.3 image from drone/images#9 as |
* Implement Branch item actions * Implement Branch item actions * Fix sorting
No description provided.