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
Support deploying with docker compose without local docker-compose #462
Conversation
compose_executors.go
Outdated
args := append(cmds, options.Args...) | ||
|
||
req := ContainerRequest{ | ||
Image: "docker/compose:1.29.2", |
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.
Wdyt about using a configurable version of compose?
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.
Agree, will do
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.
I replaced ref to function to interface ComposeExecutor
with 2 implementations.
Codecov Report
@@ Coverage Diff @@
## main #462 +/- ##
==========================================
- Coverage 65.55% 64.29% -1.27%
==========================================
Files 19 20 +1
Lines 1199 1305 +106
==========================================
+ Hits 786 839 +53
- Misses 305 354 +49
- Partials 108 112 +4
Continue to review full report at Codecov.
|
return NewDockerCompose(filePaths, identifier, LocalDockerComposeExecutor, nil, opts...) | ||
} | ||
|
||
func NewContainerizedDockerCompose(filePaths []string, identifier string, opts ...LocalDockerComposeOption) *LocalDockerCompose { |
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.
Do we still return a Local
DockerCompose? It seems weird to me to use a containerised compose method that returns a LocalDC.
I'd be open to using a dedicated DockerisedDockerCompose type, although I think could come with changes in return types and generalisation...
Of course, not a blocker for this PR, but if you are interested in following-up with more refactors to make a stable API, I'm too
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.
I started with creating dedicated DockerisedDockerCompose
type, but got too many unreasonable duplications. Using current LocalDockerCompose
with different executors is a best solution for my opinion.
IMHO: interface DockerCompose
should be removed and type LocalDockerCompose
should renamed to DockerCompose
, new interface ComposeExecutor
should be used for extensions.
Alternatively:
-
Rename type
LocalDockerCompose
toDockerComposeXYZ
. -
Define type
LocalDockerCompose
as alias ofDockerComposeXYZ
for back compatibility and mark it as deprecated:
// Deprecated: Use DockerComposeXYZ instead.
type `LocalDockerCompose` `DockerComposeXYZ`
As you can see I used existing docker-compose tests for containerized docker-compose and it is works out-of-box :)
compose_test.go
Outdated
func enumDockerComposes(filePaths []string, identifier string, executor func(compose *LocalDockerCompose), t *testing.T) { | ||
composes := []*LocalDockerCompose{ | ||
NewLocalDockerCompose(filePaths, identifier, WithLogger(TestLogger(t))), | ||
NewContainerizedDockerCompose(filePaths, identifier, WithLogger(TestLogger(t))), |
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.
I think we are using the new Dockerised compose here. Could you update the docs with examples on how to use it? There is a dedicated section for compose in the docs/features dir.
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.
Done
I replaced ref to function to interface Also I applied all |
Related to this issue: #425.
I added support conteinerised docker-compose. To do this I refectored
compose.go
and extractedComposeExecutor
which can be:ContainerizedDockerComposeExecutor
- uses docker-compose from docker container.LocalDockerComposeExecutor
- uses local docker-compose.