Deprecate docker links: services can fully replace them #366
Conversation
vdel
force-pushed
the
dev-deprecated-links
branch
3 times, most recently
from
6c1efea to
11f538d
Compare
vdel
force-pushed
the
dev-allow-external-image
branch
from
63853b1 to
5d84812
Compare
vdel
force-pushed
the
dev-deprecated-links
branch
2 times, most recently
from
21013c3 to
16ad687
Compare
thomas-riccardi
changed the base branch from
dev-allow-external-image
to
master
|
/rebuild |
thomas-riccardi
force-pushed
the
dev-deprecated-links
branch
from
16ad687 to
56a5c20
Compare
| # We link the service using its name, unless the service name has ':' in which case it is a variant | ||
| # and it is better to let the user chose the link name if needed | ||
| if self.service_name.find(':') < 0: | ||
| self.__fields__['link_name'] = self.service_name |
There was a problem hiding this comment.
if we keep this, we should document it in the field description
| # We link the service using its name, unless the service name has ':' in which case it is a variant | ||
| # and it is better to let the user chose the link name if needed | ||
| if self.service_name.find(':') < 0: | ||
| self.__fields__['link_name'] = self.service_name |
There was a problem hiding this comment.
it will possibly lead to silent link name conflict in case of multiple needed services on same service but with different env customization.
=> I suggest to not do it then.
=> maybe also add an explicit check on link name duplication somewhere?
| fi | ||
| if [ ! -z "${PROBE_PORTS}" ]; then | ||
| ROOT=$(dirname $0) | ||
| >&2 docker run --rm --link ${CONTAINER_ID}:link_to_wait -v ${ROOT}/dmake_wait_for_it:/usr/bin/dmake_wait_for_it -v ${ROOT}/dmake_wait_for_it_wrapper:/usr/bin/dmake_wait_for_it_wrapper -i ubuntu dmake_wait_for_it_wrapper "link_to_wait" "${PROBE_PORTS}" |
There was a problem hiding this comment.
nit: logs could be more informative with the real link name instead of the hardcoded link_to_wait, even if it requires passing it as an extra argument
| set -e | ||
|
|
||
| CONTAINER_ID=${1} | ||
| PROBE_PORTS=${2} |
There was a problem hiding this comment.
nit: the {} are not necessary, and are unusual for positional arguments
| return 'bash -c "%s"' % cmd | ||
|
|
||
| class DeployConfigSerializer(YAML2PipelineSerializer): | ||
| class DeployConfigSerializer(YAML2PipelineSerializer, ProbePortMixin): |
There was a problem hiding this comment.
This enables the auto port probing for all Services: this could break things.
For example test-gpu root image declare some ports, which are now auto waited-for when test-gpu is run ('dmake run test-gpu, or if test-gpuis ever added as aneeded_services`).
thomas-riccardi
left a comment
thomas-riccardi
left a comment
There was a problem hiding this comment.
Additional side effect of using real services instead of docker_links: they now inherit the dmake.yml root env, whereas with the docker_links the environments where completely isolated.
Maybe add an option to ignore the root env per service? (env_override is not enough: it just adds additional environment variables).
The current workaround would be to move all docker_links to a separate dmake.yml file (and put it a new clear directory; which hinders readability).
vdel
added
lifecycle/PAUSED
Docker links duplicate many notions and are not necessary. In order to unify how we handle the various concepts in a deployment pipeline, let's get rid of them.
Migration indications:
For each link, create a service. Use the following mapping to update the service definition:
docker_links::image_nameservices::config::docker_imagedocker_links::link_nameservices::service_namedocker_links::probe_portsservices::config::probe_portsdocker_links::envservices::config::env_overridedocker_links::volumesservices::config::volumesdocker_links::need_gpuservices::config::need_gpudocker_links::testing_optionsservices::config::docker_optsdocker_links::env_exportsservices::needed_services::env_exportsI'm not sure the last one will work, why is it injected when calling needed_services ? Anyway, I haven't changed this behavior here.