Make public functions of Project type immutable #518

glours · 2023-12-27T16:18:43Z

⚠️ Breaking changes ⚠️

What I did

Add github.com/mitchellh/copystructure to make deep copy of Project struct
Make all Project public functions which mutate Project state immutabl, returning a new instance of Project and usage a common With prefix for each of them
Replace the Project.WithProject function by a Project.ForEachProject function
Remove the unused types.set type

Related issue
https://docker.atlassian.net/browse/COMP-380

ndeloof · 2023-12-29T16:34:18Z

types/project.go

+	return newProject, nil
+}
+
+func (p *Project) projectDeepCopy() (*Project, error) {


why not just deepCopy?

Signed-off-by: Guillaume Lours <[email protected]>

milas

LGTM, some comments but nothing majorly concerning.

Two other high-level thoughts:

mitchellh is no longer doing Go dev - he recently archived a bunch of libs; I'm not super concerned since deep copying is an internal detail and doesn't pollute the API, more of an FYI
The naming & arguments of all these functions is pretty inconsistent, since this is all breaking API changes anyway, it might make sense to do a pass for consistency, e.g. make them all WithFoo() style.

milas · 2024-01-08T20:34:27Z

types/project.go

-func (p *Project) WithServices(names []string, fn ServiceFunc, options ...DependencyOption) error {
+// WithServices runs ServiceFunc on each service and dependencies according to DependencyPolicy
+// It returns a new Project instance with the changes and keep the original Project unchanged
+func (p *Project) WithServices(names []string, fn ServiceFunc, options ...DependencyOption) (*Project, error) {


This method is the only one I think is odd - the returned project option is always identical to the original.

I think it might make sense to have a pair of functions:

ForEach(func(name string, svc ServiceConfig) error) error - functional loop helper, does not copy project

MutateServices(func(name string, svc *ServiceConfig) error) (*Project, error) - clone project, allowing caller to get a mutable reference to each service in the process

Agree. As we now use WithXX fr mutating functions, this name is not relevant anymore. ForEach would be a better one. We then can have a new WithServiceFn(func(ServiceConfig) ServiceConfig) for service mutation, if relevant (afaik we don't use this in docker/compose)

milas · 2024-01-08T20:37:22Z

types/project.go

-func (p *Project) DisableService(service ServiceConfig) {
+// DisableService removes from the project model the given service and its references in all dependencies
+// It returns a new Project instance with the changes and keep the original Project unchanged
+func (p *Project) DisableService(service ServiceConfig) (*Project, error) {


minor: it's kind of weird that this one takes a ServiceConfig instead of a name like all the others (but that's not new)

milas · 2024-01-08T20:42:23Z

types/project.go

+	instance, err := copystructure.Copy(p)
+	if err != nil {
+		return nil, err
+	}


In theory, this should not fail, right? It might make sense for this to panic() to avoid carrying the error everywhere.

milas · 2024-01-08T20:46:41Z

types/project.go

-func (p *Project) ForServices(names []string, options ...DependencyOption) error {
+// ForServices restricts the project model to selected services and dependencies
+// It returns a new Project instance with the changes and keep the original Project unchanged
+func (p *Project) ForServices(names []string, options ...DependencyOption) (*Project, error) {


(see other comment re: getting rid of error from deepCopy())

I am wondering if it makes sense to ignore unknown names here, so that this method can return *Project, which would make it much more ergonomic to use: p = p.ForServices(svc).

In particular, this could allow chaining, e.g.

p.ForServices(svcs).ForEach(...)

Or we can introduce an Either type 😇 , WDYT?
edit: maybe a bad idea, this kind of structure isn't used in the Golang ecosystem.

I am wondering if it makes sense to ignore unknown names here

this would be a bug then if we silently ignore incorrect parameter being set, especially if this is "only" for code convenience.

ndeloof · 2024-01-09T07:23:46Z

mitchellh is no longer doing Go dev

.. which is sad as we rely on mapstructure for parsing. Maybe we should revisit this to only rely on yaml.Unmarshall - unfortunately go-yaml is not open to such hybrid approach and my PR didn't received much attention.
We could implement deepClone without such a reflexion framework, by plain hand-written clone code

milas · 2024-01-09T14:10:58Z

Yeah...at least mapstructure does seem to be maintained with recent commits and is very popular. In either case, I'm not super worried – we can also always fork them (or move to a community fork) if the need arises

milas

lgtm!

types/project.go

milas · 2024-01-10T16:20:29Z

I fixed the DCO status and set the option in the repo settings so it doesn't happen again with accepting PR comments:

glours · 2024-01-10T16:26:09Z

I fixed the DCO status and set the option in the repo settings so it doesn't happen again with accepting PR comments:

Arff I just push force without pulling your changes my bad 🤦

Edit: Humm as we use a copy of each service now, I think we don't need the comment anymore in fact

Signed-off-by: Guillaume Lours <[email protected]>

glours force-pushed the immutability-refactoring branch 5 times, most recently from c5a8d95 to 4182b0a Compare December 29, 2023 09:47

glours changed the title ~~Make public function of Project type immutable~~ Make public functions of Project type immutable Dec 29, 2023

glours force-pushed the immutability-refactoring branch from 4182b0a to a8f6551 Compare December 29, 2023 10:15

glours self-assigned this Dec 29, 2023

glours marked this pull request as ready for review December 29, 2023 15:32

glours requested a review from ndeloof as a code owner December 29, 2023 15:32

glours requested review from milas and ndeloof and removed request for ndeloof December 29, 2023 15:33

glours mentioned this pull request Dec 29, 2023

update compose-go to version using immutable Project functions docker/compose#11307

Merged

ndeloof reviewed Dec 29, 2023

View reviewed changes

make public functions of Project type returning a new Project object

77f5c14

Signed-off-by: Guillaume Lours <[email protected]>

glours force-pushed the immutability-refactoring branch from a8f6551 to 77f5c14 Compare December 29, 2023 19:16

glours requested a review from ndeloof December 29, 2023 19:18

ndeloof approved these changes Jan 8, 2024

View reviewed changes

milas approved these changes Jan 8, 2024

View reviewed changes

glours requested review from ndeloof and milas January 10, 2024 09:39

ndeloof approved these changes Jan 10, 2024

View reviewed changes

glours force-pushed the immutability-refactoring branch 3 times, most recently from 5ac1ba3 to 06666e2 Compare January 10, 2024 13:36

ndeloof approved these changes Jan 10, 2024

View reviewed changes

milas approved these changes Jan 10, 2024

View reviewed changes

types/project.go Outdated Show resolved Hide resolved

glours force-pushed the immutability-refactoring branch from eb908b2 to be5b2dd Compare January 10, 2024 16:24

improve Project API consistency

8d9c164

Signed-off-by: Guillaume Lours <[email protected]>

glours force-pushed the immutability-refactoring branch from be5b2dd to 8d9c164 Compare January 10, 2024 16:29

ndeloof approved these changes Jan 10, 2024

View reviewed changes

ndeloof merged commit 3d88bd1 into compose-spec:main Jan 10, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make public functions of Project type immutable #518

Make public functions of Project type immutable #518

glours commented Dec 27, 2023 •

edited

Loading

ndeloof Dec 29, 2023

glours Dec 29, 2023

milas left a comment

milas Jan 8, 2024

ndeloof Jan 10, 2024

milas Jan 8, 2024

milas Jan 8, 2024

milas Jan 8, 2024

glours Jan 9, 2024 •

edited

Loading

ndeloof Jan 9, 2024 •

edited

Loading

ndeloof commented Jan 9, 2024 •

edited

Loading

milas commented Jan 9, 2024

milas left a comment

milas commented Jan 10, 2024

glours commented Jan 10, 2024 •

edited

Loading

Make public functions of Project type immutable #518

Make public functions of Project type immutable #518

Conversation

glours commented Dec 27, 2023 • edited Loading

ndeloof Dec 29, 2023

Choose a reason for hiding this comment

glours Dec 29, 2023

Choose a reason for hiding this comment

milas left a comment

Choose a reason for hiding this comment

milas Jan 8, 2024

Choose a reason for hiding this comment

ndeloof Jan 10, 2024

Choose a reason for hiding this comment

milas Jan 8, 2024

Choose a reason for hiding this comment

milas Jan 8, 2024

Choose a reason for hiding this comment

milas Jan 8, 2024

Choose a reason for hiding this comment

glours Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

ndeloof Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

ndeloof commented Jan 9, 2024 • edited Loading

milas commented Jan 9, 2024

milas left a comment

Choose a reason for hiding this comment

milas commented Jan 10, 2024

glours commented Jan 10, 2024 • edited Loading

glours commented Dec 27, 2023 •

edited

Loading

glours Jan 9, 2024 •

edited

Loading

ndeloof Jan 9, 2024 •

edited

Loading

ndeloof commented Jan 9, 2024 •

edited

Loading

glours commented Jan 10, 2024 •

edited

Loading