Fix: use container name consistent with Docker Compose API #633

r00tu53r · 2021-12-22T07:40:08Z

Docker compose v1 (python) and v2 (Go) use
different service naming conventions. Set
service name based on the version.

Confirmed with docker compose source and tests:

https://github.com/docker/compose/blob/v2/pkg/compose/compose.go#L36

* Docker compose v1 (python) and v2 (Go) use different service naming conventions. Set service name based on the version.

elasticmachine · 2021-12-22T07:44:42Z

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Reason: Aborted from #11
Start Time: 2021-12-24T10:35:54.061+0000
Duration: 18 min 19 sec
Commit: 42416ef

Test stats 🧪

Test	Results
Failed	0
Passed	237
Skipped	0
Total	237

Steps errors

Expand to view the steps failures

`Check`

Took 5 min 33 sec . View more details here
Description: make build test-stack-command-8x check-git-clean

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

mtojek

Thanks for your contribution, Sai! I left a few comments, so let me know what do you think :)

internal/testrunner/runners/system/servicedeployer/compose.go

mtojek · 2021-12-22T09:03:58Z

internal/compose/compose.go

@@ -340,7 +345,29 @@ func (p *Project) runDockerComposeCmd(opts dockerComposeOptions) error {
 	return cmd.Run()
 }

+func (p *Project) DockerComposeVersion() (string, error) {


As this method is used only by ContainerName, do you think we could make it package-visible and access it only in ContainerName?

This way you would prevent adjusting all calling code. If we need this condition in other places, we can refactor it then.

Sounds good. I have refactored this to make it package-visible. To avoid calling docker-compose --version each time ContainerName is invoked I've stored the value in a project variable.

internal/compose/compose.go

mtojek · 2021-12-22T09:17:52Z

BTW CI reported some problems with this PR.

mtojek

Hey @r00tu53r, did you forget to push your branch? I don't see any changes applied, but comments were marked as resolved.

internal/compose/compose.go

internal/testrunner/runners/system/servicedeployer/compose.go

r00tu53r · 2021-12-23T09:52:41Z

@mtojek sorry I forgot to push the changes. I've resolved all of the above issues.

mtojek

I added a few suggestions, let me know what do you think.

mtojek · 2021-12-23T10:07:19Z

internal/compose/compose.go

@@ -340,7 +343,38 @@ func (p *Project) runDockerComposeCmd(opts dockerComposeOptions) error {
 	return cmd.Run()
 }

+func (p *Project) dockerComposeVersion() (*semver.Version, error) {
+


nit: empty line (cane be removed)

mtojek · 2021-12-23T10:14:16Z

internal/compose/compose.go

 // ContainerName method the container name for the service.
 func (p *Project) ContainerName(serviceName string) string {
-	return fmt.Sprintf("%s_%s_1", p.name, serviceName)
+	if p.dcMajorVersion == "" {


It might be cleaner to keep the ContainerName function as simple as possible:

if p.dockerComposeV1 { return fmt.Sprintf("%s_%s_1", p.name, serviceName) } return fmt.Sprintf("%s-%s-1", p.name, serviceName)

Then, the logic determining p.dockerComposeV1 can be moved to the NewProject.

dockerComposeV1 can be bool, as the Compose V2 seems to deprecate V1 in the future.

internal/compose/compose.go

mtojek · 2021-12-23T10:18:24Z

internal/compose/compose.go

-		name,
-		paths,
+		name:             name,
+		dcMajorVersion:   "",


As suggested in the other comment, dcMajorVersion can be replaced with dockerComposeV1 (bool).

Then in the NewProject, you can evaluate its value.

BTW I hope that the Project isn't created somewhere just with {}.

mtojek

Two small nit-picks are left, but it looks fine now!

Let me know what do you think about them and then I will merge this PR.

mtojek · 2021-12-23T12:20:20Z

internal/compose/compose.go

-		name,
-		paths,
+		name:             name,
+		dockerComposeV1:  true,


nit: You could also skip this property here and set it only when ve.Major == 1 or undefined. But it would the same outcome :)

Sorry I didn't quite get what you meant. Did you mean we could use the semver.Version (*Version) inside the Project and use ver.Major() in ContainerName instead ?

Currently, you're setting the property dockerComposeV1: true by default. But you could adjust it to be the other way around, so just assume:

c := Project{ name: name, composeFilePaths: paths }

and then just enable it for two cases:

if ver.Major() == 1 { c.dockerComposeV1 = true }

if err != nil { logger.Errorf("Unable to determine docker-compose version: %v. Defaulting to 1.x", err) c.dockerComposeV1 = true return &c, nil }

Let's see if I have permissions to push it here, so that we don't need to bother you anymore :)

r00tu53r · 2021-12-24T05:04:06Z

/test

mtojek · 2021-12-24T10:51:24Z

/test

mtojek · 2021-12-24T11:11:05Z

Error seems to be unrelated. I will merge this PR.

elasticmachine · 2021-12-24T11:13:52Z

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-12-24T10:51:59.175+0000
Duration: 22 min 50 sec
Commit: 42416ef

Test stats 🧪

Test	Results
Failed	0
Passed	451
Skipped	0
Total	451

Steps errors

Expand to view the steps failures

`Check`

Took 5 min 1 sec . View more details here
Description: make build test-stack-command-8x check-git-clean

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

Set service naming per docker compoose version

906267f

* Docker compose v1 (python) and v2 (Go) use different service naming conventions. Set service name based on the version.

r00tu53r requested review from mtojek and andrewkroh December 22, 2021 07:40

mtojek reviewed Dec 22, 2021

View reviewed changes

mtojek reviewed Dec 23, 2021

View reviewed changes

internal/compose/compose.go Outdated Show resolved Hide resolved

internal/compose/compose.go Outdated Show resolved Hide resolved

internal/compose/compose.go Outdated Show resolved Hide resolved

internal/testrunner/runners/system/servicedeployer/compose.go Outdated Show resolved Hide resolved

refactor code, fix linter failure

3904cfb

mtojek reviewed Dec 23, 2021

View reviewed changes

r00tu53r and others added 2 commits December 23, 2021 22:22

refactor

dc8dd27

Update internal/compose/compose.go

35ba5cf

mtojek changed the title ~~Set service naming per docker compose version~~ Fix: use container name consistent with Docker Compose API Dec 23, 2021

mtojek approved these changes Dec 23, 2021

View reviewed changes

remove extraneous newline

8a0f337

mtojek added 4 commits December 24, 2021 11:20

Adjust Project{}

a9710fe

Fix

201fd2c

nit

27c58be

Fix: make format

42416ef

mtojek merged commit ef32056 into elastic:master Dec 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: use container name consistent with Docker Compose API #633

Fix: use container name consistent with Docker Compose API #633

r00tu53r commented Dec 22, 2021

elasticmachine commented Dec 22, 2021 •

edited

Loading

Build stats

Test stats 🧪

`Check`

mtojek left a comment

mtojek Dec 22, 2021

r00tu53r Dec 23, 2021

mtojek commented Dec 22, 2021

mtojek left a comment

r00tu53r commented Dec 23, 2021 •

edited

Loading

mtojek left a comment

mtojek Dec 23, 2021

mtojek Dec 23, 2021

mtojek Dec 23, 2021

mtojek left a comment

mtojek Dec 23, 2021

r00tu53r Dec 24, 2021 •

edited

Loading

mtojek Dec 24, 2021

mtojek Dec 24, 2021

mtojek Dec 24, 2021

r00tu53r commented Dec 24, 2021

mtojek commented Dec 24, 2021

mtojek commented Dec 24, 2021

elasticmachine commented Dec 24, 2021

Build stats

Test stats 🧪

`Check`

Fix: use container name consistent with Docker Compose API #633

Fix: use container name consistent with Docker Compose API #633

Conversation

r00tu53r commented Dec 22, 2021

elasticmachine commented Dec 22, 2021 • edited Loading

❕ Build Aborted

Build stats

Test stats 🧪

Steps errors

Check

🤖 GitHub comments

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek commented Dec 22, 2021

mtojek left a comment

Choose a reason for hiding this comment

r00tu53r commented Dec 23, 2021 • edited Loading

mtojek 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

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r00tu53r Dec 24, 2021 • 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

r00tu53r commented Dec 24, 2021

mtojek commented Dec 24, 2021

mtojek commented Dec 24, 2021

elasticmachine commented Dec 24, 2021

💔 Build Failed

Build stats

Test stats 🧪

Steps errors

Check

🤖 GitHub comments

elasticmachine commented Dec 22, 2021 •

edited

Loading

`Check`

r00tu53r commented Dec 23, 2021 •

edited

Loading

r00tu53r Dec 24, 2021 •

edited

Loading

`Check`