OZ-560: Rename project,Added E2E tests and registration Opt In #1

enyachoke · 2024-09-20T05:07:22Z

This is a PR for the work in https://mekomsolutions.atlassian.net/browse/OZ-560. It introduces a number of changes to the original project;

Container registration is now opt-in
Upgrade a number of libraries
Adds E2E Tests

kdaud · 2024-09-23T09:21:36Z

e2e/container2consul.spec.ts

+  await stopDockerCompose();
+  await startDockerCompose();
+  // Pause for 5 seconds to allow nginx test containers to start
+  await new Promise(r => setTimeout(r, 3000));


Suggested change

await new Promise(r => setTimeout(r, 3000));

await new Promise(r => setTimeout(r, 5000));

@kdaud is this based on your tests locally?

Setting 3000 milliseconds equates to 3 seconds, and the comment indicates that a pause of 5 minutes is required to allow the Nginx test containers to start.

rbuisson · 2024-09-23T09:11:42Z

README.md

+  -e c2c_consul__host=localhost \
+  -e c2c_consul__port=8500 \
+  -v /var/run/docker.sock:/var/run/docker.sock \
+  mekomsolutions/container2sul


Should we keep this Docker image name?

rbuisson · 2024-09-23T09:13:24Z

README.md

+[![Build Status](https://travis-ci.org/mekomsolutions/container-to-consul.svg?branch=master)](https://travis-ci.org/mekomsolutions/container-to-consul) 
+[![codecov](https://codecov.io/gh/mekomsolutions/container-to-consul/branch/master/graph/badge.svg)](https://codecov.io/gh/mekomsolutions/container-to-consul)


Are these going to work?

Did you remove these lines?

rbuisson · 2024-09-23T09:19:58Z

README.md

+The registered ip address can be configured using a `consul.ip` label.
+
+```
+docker run --rm --name containertoConsul -ti -l consul.ip=10.0.0.1 alpine ash


Let's have the container named container-to-consul if possible.

rbuisson · 2024-09-23T09:22:16Z

README.md

+
+Container-to-Consul automatically registers and deregisters nodes for any Docker container.
+
+This project is inspired by [gliderlabs registrator](https://github.com/gliderlabs/registrator).


Isn't this based on https://github.com/benoitvidis/con-tainer2sul instead?

If so, it needs more emphasis.
What about:

This project is largely based on the original work done in con-tainer2sul by Benoît Vidis. While this fork includes significant changes and improvements, we would like to acknowledge the foundation provided by the original project, without which this version would not have been possible.

Please note that some files, variables, or other elements might still reference the original project name. These remnants will be updated over time as the project evolves, but their presence reflects the project's roots as con-tainer2sul.

rbuisson · 2024-09-23T09:24:50Z

README.md

+[![Build Status](https://travis-ci.org/mekomsolutions/container-to-consul.svg?branch=master)](https://travis-ci.org/mekomsolutions/container-to-consul) 
+[![codecov](https://codecov.io/gh/mekomsolutions/container-to-consul/branch/master/graph/badge.svg)](https://codecov.io/gh/mekomsolutions/container-to-consul)
+
+Container-to-Consul automatically registers and deregisters nodes for any Docker container.


Suggested change

Container-to-Consul automatically registers and deregisters nodes for any Docker container.

Container-to-Consul automatically registers and deregisters any Docker container as HashiCorp Consul nodes.

rbuisson · 2024-09-23T09:31:17Z

README.md

@enyachoke the doc specifies multiple commands to be run, with different option. Can those options be used simultaneously? eg, using consul.service=myservice and consul.tags=xyz

Yes they can as used in the tests

container-to-consul/e2e/docker/test-compose.yaml

Lines 4 to 10 in 52f007b

image: nginx:1-alpine

labels:

- "consul.port=80"

- "consul.tags=nginx"

- "consul.service=nginx-register"

- "consul.ip=10.0.0.1"

- "consul.register=true"

In that case, the structure of the doc is misleading.
Why would someone want to run those commands one by one like that?

I suppose it is better to simply document what the options can do, without an example docker run --rm ... which in fact will probably never be used.

In that case, the structure of the doc is misleading. Why would someone want to run those commands one by one like that?

I suppose it is better to simply document what the options can do, without an example docker run --rm ... which in fact will probably never be used.

The commands are used to explain the effect of the option and are not necessarily complete usage examples.

rbuisson · 2024-09-23T09:41:28Z

@enyachoke could you provide some instructions to test?
The Docker image mekomsolutions/container2sul does not seem to be available so how do I build it?

enyachoke · 2024-09-23T10:40:12Z

@enyachoke could you provide some instructions to test? The Docker image mekomsolutions/container2sul does not seem to be available so how do I build it?

@rbuisson This will be available after we merge see

container-to-consul/.github/workflows/build.yml

Lines 13 to 19 in 52f007b

    
           uses: mekomsolutions/shared-github-workflow/.github/workflows/docker-build-publish.yml@main 
        
           with: 
        
             download-artifacts: false 
        
             image-name: "container-to-consul" 
        
           secrets: 
        
             DOCKER_HUB_USERNAME: ${{ secrets.DOCKER_HUB_REGISTRY_USERNAME }} 
        
             DOCKER_HUB_PASSWORD: ${{ secrets.DOCKER_HUB_REGISTRY_PASSWORD }}

rbuisson · 2024-09-23T12:26:44Z

Yes I know. So what are the instructions?
docker build first? (what exact command). And then what?

rbuisson

I've pushed a change on the README to add some screenshots.
I've left some comments as well.

But it works!
Thanks, that's an awesome tool!

rbuisson · 2024-09-27T10:45:52Z

.github/workflows/build.yml

+    types: [published]
+
+jobs:
+  docker-release:


Suggested change

docker-release:

docker-publish:

rbuisson · 2024-09-27T10:50:25Z

package.json

+    "unit-tests": "istanbul cover _mocha -- --recursive --reporter spec --require should --require should-sinon"
+  },
+  "author": "Benoît Vidis",
+  "license": "Apache-2.0",


Could you add a new key for contributors:

"contributors": [ "Mekom Solutions (https://mekomsolutions.com/)" ]

rbuisson · 2024-09-27T12:40:52Z

README.md

+[![Build Status](https://travis-ci.org/mekomsolutions/container-to-consul.svg?branch=master)](https://travis-ci.org/mekomsolutions/container-to-consul) 
+[![codecov](https://codecov.io/gh/mekomsolutions/container-to-consul/branch/master/graph/badge.svg)](https://codecov.io/gh/mekomsolutions/container-to-consul)


Did you remove these lines?

rbuisson · 2024-09-30T11:52:31Z

README.md

+Running;
+
+```
+docker run --name docker-nginx -d -l consul.register=true -l consul.ip=10.0.0.1 -l consul.service=nginx-service -l consul.tags=www,api -l consul.port=80 -l consul.kv.foo=bar -l consul.kv.bar=baz  nginx


Suggested change

docker run --name docker-nginx -d -l consul.register=true -l consul.ip=10.0.0.1 -l consul.service=nginx-service -l consul.tags=www,api -l consul.port=80 -l consul.kv.foo=bar -l consul.kv.bar=baz nginx

docker run --name nginx -d \

-l consul.register=true \

-l consul.ip=10.0.0.1 \

-l consul.service=nginx \

-l consul.tags=www,api \

-l consul.port=80 \

-l consul.kv.foo=bar \

-l consul.kv.bar=baz \

nginx

rbuisson · 2024-10-02T17:20:32Z

README.md

+
+### Deregistration
+
+To deregister the container, you can simply stop it and remove it.


Is removing the container necessary to deregister? I don't think so, right?
I suggest to then remove the rm command to put it a bit after, as a note or something.

kdaud

LGTM

@rbuisson needs to do a final review of the PR.

vidiben and others added 15 commits September 20, 2024 08:02

Initial commit

586c242

LICENSE

5c3aca2

deregister non-running containers + first unit tests

27f549c

allows skipping registration

11c7921

travis

1c0151f

simplify travis

acea087

unit tests

db99469

strict mode for errors

c46ff55

strict mode for node 4.x

4df32e2

readme

c003cee

readme

6d3c50f

auto reconnect to docker

77db8eb

readme

200eb1f

fix tests

89b5260

add key/value pair support

f877b91

rbuisson self-requested a review September 23, 2024 09:09

kdaud requested changes Sep 23, 2024

View reviewed changes

rbuisson reviewed Sep 23, 2024

View reviewed changes

rbuisson requested changes Oct 2, 2024

View reviewed changes

enyachoke force-pushed the OZ-560 branch from 43b9e5b to c8170de Compare October 4, 2024 07:23

OZ-560: Rename project,Added E2E tests and registration Opt In

c89a353

enyachoke force-pushed the OZ-560 branch from c8170de to c89a353 Compare October 4, 2024 07:24

enyachoke requested review from rbuisson and kdaud October 4, 2024 07:26

kdaud approved these changes Oct 4, 2024

View reviewed changes

rbuisson approved these changes Oct 7, 2024

View reviewed changes

rbuisson merged commit 13f6f97 into mekomsolutions:main Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OZ-560: Rename project,Added E2E tests and registration Opt In #1

OZ-560: Rename project,Added E2E tests and registration Opt In #1

enyachoke commented Sep 20, 2024 •

edited

Loading

kdaud Sep 23, 2024

enyachoke Sep 23, 2024

kdaud Sep 23, 2024

rbuisson Sep 23, 2024

rbuisson Sep 23, 2024

rbuisson Sep 27, 2024

rbuisson Sep 23, 2024 •

edited

Loading

rbuisson Sep 23, 2024

rbuisson Sep 23, 2024

rbuisson Sep 23, 2024

rbuisson Sep 23, 2024

enyachoke Sep 23, 2024

rbuisson Sep 23, 2024 •

edited

Loading

enyachoke Sep 27, 2024

rbuisson commented Sep 23, 2024 •

edited

Loading

enyachoke commented Sep 23, 2024 •

edited

Loading

rbuisson commented Sep 23, 2024

rbuisson left a comment

rbuisson Sep 27, 2024

rbuisson Sep 27, 2024

rbuisson Sep 27, 2024

rbuisson Sep 30, 2024

rbuisson Oct 2, 2024

kdaud left a comment

	await new Promise(r => setTimeout(r, 3000));
	await new Promise(r => setTimeout(r, 5000));

		[![Build Status](https://travis-ci.org/mekomsolutions/container-to-consul.svg?branch=master)](https://travis-ci.org/mekomsolutions/container-to-consul)
		[![codecov](https://codecov.io/gh/mekomsolutions/container-to-consul/branch/master/graph/badge.svg)](https://codecov.io/gh/mekomsolutions/container-to-consul)


		Container-to-Consul automatically registers and deregisters nodes for any Docker container.

		This project is inspired by [gliderlabs registrator](https://github.com/gliderlabs/registrator).

	image: nginx:1-alpine
	labels:
	- "consul.port=80"
	- "consul.tags=nginx"
	- "consul.service=nginx-register"
	- "consul.ip=10.0.0.1"
	- "consul.register=true"

-docker run --name docker-nginx -d -l consul.register=true -l consul.ip=10.0.0.1 -l consul.service=nginx-service -l consul.tags=www,api -l consul.port=80 -l consul.kv.foo=bar -l consul.kv.bar=baz  nginx
+docker run --name nginx -d \
+ -l consul.register=true \
+  -l consul.ip=10.0.0.1 \
+   -l consul.service=nginx \
+    -l consul.tags=www,api \
+     -l consul.port=80 \
+      -l consul.kv.foo=bar \
+       -l consul.kv.bar=baz \
+        nginx


		### Deregistration

		To deregister the container, you can simply stop it and remove it.

OZ-560: Rename project,Added E2E tests and registration Opt In #1

OZ-560: Rename project,Added E2E tests and registration Opt In #1

Conversation

enyachoke commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuisson Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuisson Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuisson commented Sep 23, 2024 • edited Loading

enyachoke commented Sep 23, 2024 • edited Loading

rbuisson commented Sep 23, 2024

rbuisson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdaud left a comment

Choose a reason for hiding this comment

enyachoke commented Sep 20, 2024 •

edited

Loading

rbuisson Sep 23, 2024 •

edited

Loading

rbuisson Sep 23, 2024 •

edited

Loading

rbuisson commented Sep 23, 2024 •

edited

Loading

enyachoke commented Sep 23, 2024 •

edited

Loading