Skip to content
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

Add ability to modify docker-compose configuration #722

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rmccue
Copy link
Member

@rmccue rmccue commented Aug 20, 2024

Adds the ability for any Composer package (including the root/project itself) to modify the docker-compose configuration, making it possible for modules to add containers or alter the existing configuration.

This can be used for better modularity and conditional behaviour, such as for optional features like Elasticsearch or Node.js. It also provides the ability for users to customise Docker behaviour fairly directly in their project.

My plan is to transition the ES + Kibana containers to the Enhanced Search package, and then eventually someday remove it from the default bundle, allowing a clearer opt-in for customers who have actually bought it. PR for that incoming soon.

Copy link
Contributor

@mikelittle mikelittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions.
Also, the lint issues will need fixes (mostly missing docs).

@mikelittle
Copy link
Contributor

I like the idea of this.
It might be an opportunity to stop using the implicit dependency and networking configuration of links. Instead, we should use explicit depends_on and networks attributes.

@CodeProKid
Copy link

@rmccue Just noting that we'd love to have the ability to do modify the docker compose from our own project! Our immediate use case for this would be to prefix all of the image names in the generated docker-compose file with our internal docker hub proxy URL so they get pulled from there instead of directly from docker hub which will prevent us from running into docker hub API limits in CI.

It looks like we'd be able to achieve this quite easily with the way this feature has been designed 😃

@rmccue rmccue requested a review from mikelittle October 17, 2024 10:36
@mikelittle
Copy link
Contributor

I did test this locally with the ES example. It highlighted a weird error that I think it is a dependency issue, though it doesn't make sense.
With this branch and the ES PR checked out, I get the following error on startup:

$ composer server start
Starting...
PHP Fatal error:  Declaration of Symfony\Component\Console\Input\ArrayInput::hasParameterOption(array|string $values, bool $onlyParams = false): bool must be compatible with Symfony\Component\Console\Input\InputInterface::hasParameterOption($values, bool $onlyParams = false) in /Users/mikelittle/sandbox/02-altis/product-dev/vendor/symfony/console/Input/ArrayInput.php on line 50

Fatal error: Declaration of Symfony\Component\Console\Input\ArrayInput::hasParameterOption(array|string $values, bool $onlyParams = false): bool must be compatible with Symfony\Component\Console\Input\InputInterface::hasParameterOption($values, bool $onlyParams = false) in /Users/mikelittle/sandbox/02-altis/product-dev/vendor/symfony/console/Input/ArrayInput.php on line 50

Grep-ing for the function gets me three hits—all correct:

$ grep -r 'hasParameterOption'
... skipping calls
vendor/symfony/console/Input/ArrayInput.php:    public function hasParameterOption(string|array $values, bool $onlyParams = false): bool
vendor/symfony/console/Input/ArgvInput.php:    public function hasParameterOption(string|array $values, bool $onlyParams = false): bool
vendor/symfony/console/Input/InputInterface.php:    public function hasParameterOption(string|array $values, bool $onlyParams = false): bool;

So far, I made it work by specifying "symfony/console": "5.4" as a requirement in dev-tools. I've not committed this yet, But will see if the problem shows up elsewhere.

However, this issue apart, the code did work. I could enable/disable the Elastic Search container from the Enhanced Search module.

@joehoyle
Copy link
Member

+1 from me on this approach

@joehoyle
Copy link
Member

@mikelittle I got the same issue as you, it appears that it's actually conflicting with a version of Console that is in composer. In my case Input / InputInterface is declared in phar:///opt/homebrew/Cellar/composer/2.6.5/bin/composer/vendor/symfony/console/Input/Input.php. it's not clear to my why ArrayInput isn't also getting loaded from the composer's deps instead, maybe the class-loader order is unspecified?

@joehoyle
Copy link
Member

So, Composer (2.6.5) at least is bundling Symfony Console 5.4.28.0, whereas my altis install is using 6.2.x-dev, composer why symfony/console shows:

codeception/codeception 5.1.2      requires  symfony/console (>=4.4.24 <8.0)                                                  
psy/psysh               0.12.x-dev requires  symfony/console (^7.0 || ^6.0 || ^5.0 || ^4.0 || ^3.4)                           
psy/psysh               0.12.x-dev conflicts symfony/console (4.4.37 || 5.3.14 || 5.3.15 || 5.4.3 || 5.4.4 || 6.0.3 || 6.0.4) 
symfony/var-dumper      6.4.x-dev  conflicts symfony/console (<5.4)                                                           
symfony/yaml            6.3.x-dev  conflicts symfony/console (<5.4) 

I don't know to what degree the composer dependencies are taken into account when resolving versions...

@joehoyle
Copy link
Member

joehoyle commented Feb 17, 2025

It appears composer doesn't account at all for it's own deps, so it's relatively easy to get a version conflict between a project's deps and ones bundled in composer.phar. I'm not sure exactly how that should be handled. I'm assuming this is why we don't specify symfony/console as a dep of local-server, but it is getting loaded in via dev-tools, so as @mikelittle points out, sticking dev tools to symfony/console": "5.4" should resolve this. However, I don't know if other versions of the composer binary will include different major versions of console.

rmccue and others added 5 commits February 27, 2025 14:41
This adds the ability for modules to extend the docker-compose.yml
configuration with sidecar containers (or override existing
configuration).
Co-authored-by: Konstantin Kovshenin <[email protected]>
@mikelittle mikelittle force-pushed the composable-containers branch from 279db53 to 03112d9 Compare February 27, 2025 14:49
@mikelittle
Copy link
Contributor

Rebased against master and force pushed.

This fixes the version of symfony/console to 5.4.
This stops a conflict with the version of symfony/console contained within composer itself.
Apparently, when running external compose commands, the version of symfony/console picked up is the one inside composer.
@mikelittle mikelittle requested a review from kovshenin February 27, 2025 16:58
Copy link
Contributor

@mikelittle mikelittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking good now and ready to go.

@mikelittle mikelittle requested review from wisyhambolu and ferschubert-hm and removed request for kovshenin February 28, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer advocacy Developer Advocacy related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants