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

docs: add note about order of nginx-php service types #3891

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shreddedbacon
Copy link
Member

General Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for inclusion in changelog

Database Migrations

  • If your PR contains a database migation, it MUST be the latest in date order alphabetically

Description

Document that the nginx-php service requires that the nginx service is defined before the php service in the docker-compose file

@smlx
Copy link
Member

smlx commented Mar 4, 2025

This is a bit confusing because YAML maps don't have an order. Maybe this note could be clarified?

@shreddedbacon
Copy link
Member Author

This is a bit confusing because YAML maps don't have an order. Maybe this note could be clarified?

When we parse the docker compose file in the build-deploy-tool, we retain the order that the services are defined in the YAML. This is done because prior to the rewrite in Go, we used shyaml to loop over the services when calculating them for helm, and this required that for the nginx-php service type, that the php service was defined after nginx.

@smlx
Copy link
Member

smlx commented Mar 4, 2025

The YAML spec says:

To serialize a mapping, it is necessary to impose an ordering on its keys. This order is a serialization detail and should not be used when composing the representation graph (and hence for the preservation of application data). In every case where node order is significant, a sequence must be used.

So the behaviour you describe seems like a bug?

@tobybellwood
Copy link
Member

tobybellwood commented Mar 4, 2025

So the behaviour you describe seems like a bug?

Yes - just another one of the historic YAML bugs we've been dealing with (or working around) for years now - but as it's only really just bitten us, hence the documentation.

When we introduce the full ability to link any containers together in a pod, this will be more explicitly defined (ie using named services instead of an implied connection), but for the thousands of sites already out there, this has to be retained for backward compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants