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

chore: sync relevant changes from api-platform template #3255

Merged
merged 5 commits into from
Feb 24, 2023

Conversation

usu
Copy link
Member

@usu usu commented Feb 5, 2023

Related to #3253

I went through the changes of the official api-platform template and tried to sync relevant changes to our repo.

All upstream changes as a reference:
api-platform/api-platform@d059b22...7642f86

Currently excluded:

Appreciate some careful local testing.

@usu usu added the deploy! Creates a feature branch deployment for this PR label Feb 5, 2023
@github-actions
Copy link

github-actions bot commented Feb 5, 2023

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

WORKDIR /srv/api

# php extensions installer: https://github.com/mlocati/docker-php-extension-installer
COPY --from=mlocati/php-extension-installer --link /usr/bin/install-php-extensions /usr/local/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

For the --link feature to work, you need to enable buildkit on your machine.
https://docs.docker.com/build/buildkit/

to add this to our Dockerfile, we also need to add a link to the installation notes to the documentation.
Specially that you need to use cli build with docker compose that buildkit is used:
https://stackoverflow.com/questions/58592259/how-do-you-enable-buildkit-with-docker-compose

The rebuild is a lot faster, cool

Copy link
Member Author

Choose a reason for hiding this comment

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

I this specific to Linux? The documentation says that BuildKit is enabled by default for Docker Desktop, hence it worked for me out of the box.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is specific for the docker-ce (Community Edition).
With Docker Desktop it works out of the box. Github actions seems to have enabled buildkit already in the docker daemon.

Copy link
Member

Choose a reason for hiding this comment

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

Building worked for me out of the box on Linux..? If this doesn't work everywhere, do we need to change something in the setup documentation? Or is it just a matter of a new enough Docker CE version?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that docker compose build uses docker/compose@v2 which always uses buildkit by default.
Whereas docker-compose build uses docker/compose@v1, in which case either buildkit need to be enabled (automatically by Docker Desktop or manually via env variable) or by aliasing docker-compose to docker compose (which can be enabled in Docker Desktop settings).

So my naive understanding would be that this should always work out of the box when using docker compose. But cannot test on my side, as I'm using Docker Desktop.

Copy link
Member

Choose a reason for hiding this comment

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

Hm but I am pretty sure that I am using it as docker-compose, since I have an alias dcb for docker-compose build on my system...
@BacLuc can you reproduce the problem consistently?

@BacLuc
Copy link
Contributor

BacLuc commented Feb 9, 2023

Else everything works fine, thank you

@BacLuc BacLuc added deploy! Creates a feature branch deployment for this PR and removed deploy! Creates a feature branch deployment for this PR labels Feb 20, 2023
@usu usu requested a review from pmattmann February 22, 2023 20:07
@@ -7,6 +7,6 @@ doctrine_migrations:
# as migrations classes should NOT be autoloaded
'DoctrineMigrations': '%kernel.project_dir%/migrations/schema'
'DataMigrations': '%kernel.project_dir%/migrations/%data_migrations_dir%'
enable_profiler: '%kernel.debug%'
enable_profiler: false
Copy link
Member

Choose a reason for hiding this comment

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

Is really both this and the collect_serializer_data: false in web_profiler.yml necessary?

Copy link
Member Author

@usu usu Feb 23, 2023

Choose a reason for hiding this comment

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

doctrine_migrations:enable_profiler
Didn't know what this is used for, but it seems there's a tab for doctrine migrations in the symfony debug panel. Never used myself, but it actually looks like this could be useful. So I'm ok to enable this again. In the api-platform upstream repo this was disabled in commit api-platform/api-platform@6fd5050, but there's no further description, on why this was disabled.

framework:profiler:collect_serializer_data
This is related to a new functionality in Symfony 6.1 which displays serializer info in the debug panel. Default value is false, upstream repo has changed to true. Having this enabled conflicts with our implementation of TranslationConstraintViolationListNormalizer. Before we can enable this, further investigation is needed to figure out what exactly breaks (also see initial header description of this PR).

WORKDIR /srv/api

# php extensions installer: https://github.com/mlocati/docker-php-extension-installer
COPY --from=mlocati/php-extension-installer --link /usr/bin/install-php-extensions /usr/local/bin/
Copy link
Member

Choose a reason for hiding this comment

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

Building worked for me out of the box on Linux..? If this doesn't work everywhere, do we need to change something in the setup documentation? Or is it just a matter of a new enough Docker CE version?

@usu usu added the Meeting Discuss Am nächsten Core-Meeting besprechen label Feb 24, 2023
@pmattmann pmattmann removed the Meeting Discuss Am nächsten Core-Meeting besprechen label Feb 24, 2023
@pmattmann pmattmann merged commit 7a21fe0 into ecamp:devel Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants