Skip to content

Comments

make Services a map[string]ServiceConfig to reflect yaml structure#483

Merged
ndeloof merged 1 commit intocompose-spec:mainfrom
ndeloof:service_map
Nov 21, 2023
Merged

make Services a map[string]ServiceConfig to reflect yaml structure#483
ndeloof merged 1 commit intocompose-spec:mainfrom
ndeloof:service_map

Conversation

@ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Nov 6, 2023

the docker/cli compose parser we inherited as compose-go used a []ServiceConfig to map to yaml services. I'm not sure what was the reason for this design decision, and propose we adopt a natural map[string]ServiceConfig type.
For convenience, ServiceConfig.Name is set to map key

@ndeloof ndeloof force-pushed the service_map branch 4 times, most recently from 793fb23 to c4a2646 Compare November 6, 2023 15:19
@ndeloof ndeloof marked this pull request as ready for review November 6, 2023 15:56
@ndeloof ndeloof requested review from glours and laurazard and removed request for glours November 6, 2023 15:56
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM, just one point of concern where we're returning p.Services directly

Beyond that, we should think about our iteration patterns to avoid weirdness from random Go map iteration order

Comment on lines +122 to +124
if len(names) == 0 {
return p.Services, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make a copy of the map here – if a caller ends up mutating the result, they'll be unintentionally changing the project

Copy link
Collaborator Author

@ndeloof ndeloof Nov 21, 2023

Choose a reason for hiding this comment

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

This issue we already have with networks/volumes/secrets/configs
A global "immutability" refactoring is under consideration, but will come as a future effort
Please note if we try to encapsulate the map nature of this, we will have to manage the yaml unmarshaling, so we could consider to offer "mutation-safe" iterator Services.Each(func(name, ServiceConfig)), but won't preserve third-party to use a plain range. Please also note mutating the model could be required by user

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof merged commit 5bf1d3f into compose-spec:main Nov 21, 2023
@ndeloof ndeloof deleted the service_map branch November 21, 2023 08:33
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.

2 participants