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

Make public functions of Project type immutable #518

Merged
merged 2 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ require (

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f // indirect
github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
Expand Down
4 changes: 4 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@ github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/mattn/go-shellwords v1.0.12 h1:M2zGm7EW6UQJvDeQxo4T51eKPurbeFbe8WtebGE2xrk=
github.com/mattn/go-shellwords v1.0.12/go.mod h1:EZzvwXDESEeg03EKmM+RmDnNOPKG4lLtQsUlTZDWQ8Y=
github.com/mitchellh/copystructure v1.2.0 h1:vpKXTN4ewci03Vljg/q9QvCGUDttBOGBIa15WveJJGw=
github.com/mitchellh/copystructure v1.2.0/go.mod h1:qLl+cE2AmVv+CoeAwDPye/v+N2HKCj9FbZEVFJRxO9s=
github.com/mitchellh/mapstructure v1.5.0 h1:jeMsZIYE/09sWLaz43PL7Gy6RuMjD2eJVyuac5Z2hdY=
github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo=
github.com/mitchellh/reflectwalk v1.0.2 h1:G2LzWKi524PWgd3mLHV8Y5k7s6XUvT0Gef6zxSIeXaQ=
github.com/mitchellh/reflectwalk v1.0.2/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw=
github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
Expand Down
6 changes: 4 additions & 2 deletions loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,10 +451,12 @@ func load(ctx context.Context, configDetails types.ConfigDetails, opts *Options,
}
}

project.ApplyProfiles(opts.Profiles)
if project, err = project.ApplyProfiles(opts.Profiles); err != nil {
return nil, err
}

if !opts.SkipResolveEnvironment {
err := project.ResolveServicesEnvironment(opts.discardEnvFiles)
project, err = project.ResolveServicesEnvironment(opts.discardEnvFiles)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion loader/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2131,7 +2131,7 @@ func TestLoadServiceWithEnvFile(t *testing.T) {
},
},
}
err = p.ResolveServicesEnvironment(false)
p, err = p.ResolveServicesEnvironment(false)
assert.NilError(t, err)
service, err := p.GetService("test")
assert.NilError(t, err)
Expand Down
4 changes: 2 additions & 2 deletions types/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

func Test_WithServices(t *testing.T) {
p := Project{
p := &Project{
Services: Services{
"service_1": ServiceConfig{
Name: "service_1",
Expand Down Expand Up @@ -54,7 +54,7 @@ func Test_WithServices(t *testing.T) {
return nil
}

err := p.WithServices(nil, fn)
p, err := p.WithServices(nil, fn)
assert.NilError(t, err)
assert.DeepEqual(t, order, []string{"service_2", "service_3", "service_1"})
}
Expand Down
154 changes: 104 additions & 50 deletions types/project.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,16 @@ import (
"github.com/compose-spec/compose-go/v2/dotenv"
"github.com/compose-spec/compose-go/v2/utils"
"github.com/distribution/reference"
"github.com/mitchellh/copystructure"
godigest "github.com/opencontainers/go-digest"
"github.com/pkg/errors"
"golang.org/x/sync/errgroup"
"gopkg.in/yaml.v3"
)

// Project is the result of loading a set of compose files
// Since v2, Project are managed as immutable objects.
// Each public functions which mutate Project state now return a copy of the original Project with the expected changes.
type Project struct {
Name string `yaml:"name,omitempty" json:"name,omitempty"`
WorkingDir string `yaml:"-" json:"-"`
Expand Down Expand Up @@ -185,13 +188,19 @@ func (p *Project) AllServices() Services {

type ServiceFunc func(name string, service ServiceConfig) error

// WithServices run ServiceFunc on each service and dependencies according to DependencyPolicy
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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

newProject, err := p.deepCopy()
if err != nil {
return nil, err
}
if len(options) == 0 {
// backward compatibility
options = []DependencyOption{IncludeDependencies}
}
return p.withServices(names, fn, map[string]bool{}, options, map[string]ServiceDependency{})
err = newProject.withServices(names, fn, map[string]bool{}, options, map[string]ServiceDependency{})
return newProject, err
}

type withServicesOptions struct {
Expand Down Expand Up @@ -291,53 +300,69 @@ func (s ServiceConfig) HasProfile(profiles []string) bool {
}

// ApplyProfiles disables service which don't match selected profiles
func (p *Project) ApplyProfiles(profiles []string) {
// It returns a new Project instance with the changes and keep the original Project unchanged
func (p *Project) ApplyProfiles(profiles []string) (*Project, error) {
newProject, err := p.deepCopy()
if err != nil {
return nil, err
}
for _, p := range profiles {
if p == "*" {
return
return newProject, nil
}
}
enabled := Services{}
disabled := Services{}
for name, service := range p.AllServices() {
for name, service := range newProject.AllServices() {
if service.HasProfile(profiles) {
enabled[name] = service
} else {
disabled[name] = service
}
}
p.Services = enabled
p.DisabledServices = disabled
p.Profiles = profiles
newProject.Services = enabled
newProject.DisabledServices = disabled
newProject.Profiles = profiles
return newProject, nil
}

// EnableServices ensure services are enabled and activate profiles accordingly
func (p *Project) EnableServices(names ...string) error {
// EnableServices ensures services are enabled and activate profiles accordingly
// It returns a new Project instance with the changes and keep the original Project unchanged
func (p *Project) EnableServices(names ...string) (*Project, error) {
newProject, err := p.deepCopy()
if err != nil {
return nil, err
}
if len(names) == 0 {
return nil
return newProject, nil
}

profiles := append([]string{}, p.Profiles...)
for _, name := range names {
if _, ok := p.Services[name]; ok {
if _, ok := newProject.Services[name]; ok {
// already enabled
continue
}
service := p.DisabledServices[name]
profiles = append(profiles, service.Profiles...)
}
p.ApplyProfiles(profiles)
newProject, err = newProject.ApplyProfiles(profiles)
if err != nil {
return newProject, err
}

return p.ResolveServicesEnvironment(true)
return newProject.ResolveServicesEnvironment(true)
}

// WithoutUnnecessaryResources drops networks/volumes/secrets/configs that are not referenced by active services
func (p *Project) WithoutUnnecessaryResources() {
// It returns a new Project instance with the changes and keep the original Project unchanged
func (p *Project) WithoutUnnecessaryResources() (*Project, error) {
newProject, err := p.deepCopy()
requiredNetworks := map[string]struct{}{}
requiredVolumes := map[string]struct{}{}
requiredSecrets := map[string]struct{}{}
requiredConfigs := map[string]struct{}{}
for _, s := range p.Services {
for _, s := range newProject.Services {
for k := range s.Networks {
requiredNetworks[k] = struct{}{}
}
Expand Down Expand Up @@ -366,31 +391,32 @@ func (p *Project) WithoutUnnecessaryResources() {
networks[k] = value
}
}
p.Networks = networks
newProject.Networks = networks

volumes := Volumes{}
for k := range requiredVolumes {
if value, ok := p.Volumes[k]; ok {
volumes[k] = value
}
}
p.Volumes = volumes
newProject.Volumes = volumes

secrets := Secrets{}
for k := range requiredSecrets {
if value, ok := p.Secrets[k]; ok {
secrets[k] = value
}
}
p.Secrets = secrets
newProject.Secrets = secrets

configs := Configs{}
for k := range requiredConfigs {
if value, ok := p.Configs[k]; ok {
configs[k] = value
}
}
p.Configs = configs
newProject.Configs = configs
return newProject, err
}

type DependencyOption func(options *withServicesOptions)
Expand All @@ -407,25 +433,26 @@ func IgnoreDependencies(options *withServicesOptions) {
options.dependencyPolicy = ignoreDependencies
}

// ForServices restrict the project model to selected services and dependencies
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) {
Copy link
Member

Choose a reason for hiding this comment

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

(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(...)

Copy link
Collaborator Author

@glours glours Jan 9, 2024

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@ndeloof ndeloof Jan 9, 2024

Choose a reason for hiding this comment

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

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.

if len(names) == 0 {
// All services
return nil
return p.deepCopy()
}

set := utils.NewSet[string]()
err := p.WithServices(names, func(name string, service ServiceConfig) error {
newProject, err := p.WithServices(names, func(name string, service ServiceConfig) error {
set.Add(name)
return nil
}, options...)
if err != nil {
return err
return nil, err
}

// Disable all services which are not explicit target or dependencies
enabled := Services{}
for name, s := range p.Services {
for name, s := range newProject.Services {
if _, ok := set[name]; ok {
// remove all dependencies but those implied by explicitly selected services
dependencies := s.DependsOn
Expand All @@ -437,32 +464,46 @@ func (p *Project) ForServices(names []string, options ...DependencyOption) error
s.DependsOn = dependencies
enabled[name] = s
} else {
p.DisableService(s)
if newProject, err = newProject.DisableService(s); err != nil {
return nil, err
}
}
}
p.Services = enabled
return nil
newProject.Services = enabled
return newProject, nil
}

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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)

newProject, err := p.deepCopy()
if err != nil {
return nil, err
}
// We should remove all dependencies which reference the disabled service
for i, s := range p.Services {
for i, s := range newProject.Services {
if _, ok := s.DependsOn[service.Name]; ok {
delete(s.DependsOn, service.Name)
p.Services[i] = s
newProject.Services[i] = s
}
}
delete(p.Services, service.Name)
if p.DisabledServices == nil {
p.DisabledServices = Services{}
if newProject.DisabledServices == nil {
newProject.DisabledServices = Services{}
}
p.DisabledServices[service.Name] = service
newProject.DisabledServices[service.Name] = service
return newProject, err
}

// ResolveImages updates services images to include digest computed by a resolver function
func (p *Project) ResolveImages(resolver func(named reference.Named) (godigest.Digest, error)) error {
// It returns a new Project instance with the changes and keep the original Project unchanged
func (p *Project) ResolveImages(resolver func(named reference.Named) (godigest.Digest, error)) (*Project, error) {
newProject, err := p.deepCopy()
if err != nil {
return nil, err
}
eg := errgroup.Group{}
for i, s := range p.Services {
for i, s := range newProject.Services {
idx := i
service := s

Expand All @@ -488,11 +529,11 @@ func (p *Project) ResolveImages(resolver func(named reference.Named) (godigest.D
}

service.Image = named.String()
p.Services[idx] = service
newProject.Services[idx] = service
return nil
})
}
return eg.Wait()
return newProject, eg.Wait()
}

// MarshalYAML marshal Project into a yaml tree
Expand Down Expand Up @@ -533,10 +574,15 @@ func (p *Project) MarshalJSON() ([]byte, error) {
return json.Marshal(m)
}

// ResolveServicesEnvironment parse env_files set for services to resolve the actual environment map for services
func (p Project) ResolveServicesEnvironment(discardEnvFiles bool) error {
for i, service := range p.Services {
service.Environment = service.Environment.Resolve(p.Environment.Resolve)
// ResolveServicesEnvironment parses env_files set for services to resolve the actual environment map for services
// It returns a new Project instance with the changes and keep the original Project unchanged
func (p Project) ResolveServicesEnvironment(discardEnvFiles bool) (*Project, error) {
newProject, err := p.deepCopy()
if err != nil {
return nil, err
}
for i, service := range newProject.Services {
service.Environment = service.Environment.Resolve(newProject.Environment.Resolve)

environment := MappingWithEquals{}
// resolve variables based on other files we already parsed, + project's environment
Expand All @@ -545,24 +591,24 @@ func (p Project) ResolveServicesEnvironment(discardEnvFiles bool) error {
if ok && v != nil {
return *v, ok
}
return p.Environment.Resolve(s)
return newProject.Environment.Resolve(s)
}

for _, envFile := range service.EnvFiles {
if _, err := os.Stat(envFile.Path); os.IsNotExist(err) {
if envFile.Required {
return errors.Wrapf(err, "env file %s not found", envFile.Path)
return nil, errors.Wrapf(err, "env file %s not found", envFile.Path)
}
continue
}
b, err := os.ReadFile(envFile.Path)
if err != nil {
return errors.Wrapf(err, "failed to load %s", envFile.Path)
return nil, errors.Wrapf(err, "failed to load %s", envFile.Path)
}

fileVars, err := dotenv.ParseWithLookup(bytes.NewBuffer(b), resolve)
if err != nil {
return errors.Wrapf(err, "failed to read %s", envFile.Path)
return nil, errors.Wrapf(err, "failed to read %s", envFile.Path)
}
environment.OverrideBy(Mapping(fileVars).ToMappingWithEquals())
}
Expand All @@ -572,7 +618,15 @@ func (p Project) ResolveServicesEnvironment(discardEnvFiles bool) error {
if discardEnvFiles {
service.EnvFiles = nil
}
p.Services[i] = service
newProject.Services[i] = service
}
return nil
return newProject, nil
}

func (p *Project) deepCopy() (*Project, error) {
instance, err := copystructure.Copy(p)
if err != nil {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

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

return instance.(*Project), nil
}
Loading
Loading