use compose-go to load and parse compose files #4863

glours · 2024-02-08T17:06:48Z

- What I did
Use compose-go to parse, interpolate and merge Compose configurations.
As this library, under active maintenance and development by the Compose team, is up to date with the recent evolutions of the Compose Specification it will remove the potential divergence between the 2 code base, make the maintenance and evolution easier for everyone.
As a side effect, it would remove the necessity of using the version attribute. It disturbs users, the official Docker message is: "You don't need the version attribute anymore in your Compose files" and the CLI continue to introduce new versions of compose v3 file format

- How I did it
Import the compose-go library, removed the duplicated code for structs, keep only the specific checks done on forbidden, unsupported and deprecated properties on the parsing part.
Remove the interpolation and merging functions and let compose-go doing the job
Add the specific mapping for x-cluster-spec extension attribut to match the ClusterVolumeSpec struct
Remove all the duplicated tests already done on the compose-go side

- How to verify it
Run tests
Use average Compose configurations and check we're still producing the same output than the current version

- Description for the changelog

Use compose-go to manage Compose configuration files

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2024-02-08T19:51:58Z

Codecov Report

Merging #4863 (6abad62) into master (c986d09) will decrease coverage by 1.23%.
Report is 16 commits behind head on master.
The diff coverage is 65.74%.

❗ Current head 6abad62 differs from pull request most recent head 21487c5. Consider uploading reports for the commit 21487c5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4863      +/-   ##
==========================================
- Coverage   61.31%   60.09%   -1.23%     
==========================================
  Files         287      283       -4     
  Lines       20058    19131     -927     
==========================================
- Hits        12298    11496     -802     
+ Misses       6867     6782      -85     
+ Partials      893      853      -40

Radiergummi · 2024-02-12T17:06:10Z

You have NO idea how much this improves my job. Merci beaucoup 🫶

Leopere · 2024-02-14T04:03:39Z

You have NO idea how much this improves my job. Merci beaucoup 🫶

Silver plates Yeah no this is pretty wonderful if/when it get's accepted.

Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>

Go's `net` package [will unlink][1] for us, as long as we used listen to create the Unix socket. Go will even skip the unlink when the socket appears to be abstract (start with a NUL, represented by an @), though we must be cautious to only create sockets with an abstract address on platforms that actually support it -- this caused [several][2] [bugs][3] before. [1]: https://pkg.go.dev/net#UnixListener.SetUnlinkOnClose [2]: docker#4783 [3]: docker#4863 Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

Go's `net` package [will unlink][1] for us, as long as we used Listen & friends to create the Unix socket. Go will even skip the unlink when the socket appears to be abstract (starts with a NUL, represented by an @), though we must be cautious to only create sockets with an abstract address on platforms that actually support it -- this caused [several][2] [bugs][3] before. [1]: https://pkg.go.dev/net#UnixListener.SetUnlinkOnClose [2]: docker#4783 [3]: docker#4863 Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

Benehiko

I don't have all the context of the impact these changes will bring but I thought that giving a code review might help push this forward :)

Benehiko · 2024-03-25T09:08:01Z

cli/command/stack/config.go


+	specLoader "github.com/compose-spec/compose-go/v2/loader"
+	compose "github.com/compose-spec/compose-go/v2/types"


nit: keep it consistent with the other import names. Either stick with compose or composetypes.

Benehiko · 2024-03-25T13:19:14Z

cli/compose/convert/service.go

 	var o []string

+	if spec == nil {
+		return nil, nil


Although this is an internal function I don't think we should return a nil error and nil value here.

Suggested change

return nil, nil

return nil, errors.New("spec is nil")

Benehiko · 2024-03-25T13:23:56Z

cli/compose/convert/service.go

+	for hostName, hostIPs := range extraHosts {
+		for _, hostIP := range hostIPs {
+			hosts = append(hosts, hostIP+" "+hostName)
 		}
 	}
+	slices.Sort(hosts)


"IP-address hostname(s)". The original order of mappings is preserved.

Since this function mentions it keeps the order of hosts, I'm a bit skeptical of this change. Wouldn't sorting the slice here potentially change the original order of the hosts? How do we know if the host mappings were sorted in the first place?

If we do intend to break the original contract, we should then also update the comments on the function to match the new behavior of the function.

Benehiko · 2024-03-25T13:40:36Z

cli/compose/loader/loader.go

+		r := reflect.ValueOf(service)
+		for property, value := range types.UnsupportedProperties {
+			f := reflect.Indirect(r).FieldByName(property)
+			if f.IsValid() && !f.IsZero() && !slices.Contains(unsupported, value) {
+				unsupported = append(unsupported, value)


I'm always a bit skeptical of using reflect. Although the previous solution also seemed quite messy, I'm not sure if this one is better with reflect. A simple if type check might do:

if ok, f := service.(map[string]any); !ok { return errors.New("not supported") }

Benehiko · 2024-03-25T13:48:13Z

cli/compose/loader/loader.go

+		r := reflect.ValueOf(service)
+		for property, pair := range propertyMap {
+			f := reflect.Indirect(r).FieldByName(property)
+			if f.IsValid() && !f.IsZero() {
+				output[pair.Key()] = pair.Value()
 			}
 		}


I'm here also not sure if reflect is a good approach.

Benehiko · 2024-03-25T13:58:28Z

cli/command/stack/config.go

+	for _, f := range configFiles.ConfigFiles {
+		if f.Filename != "" {
+			dir = filepath.Base(f.Filename)
+			break
+		}
+	}


why do we need to iterate over the config files here? I don't know much about this part of the code, but would like to know what problem it solves. For example, In which cases would the filename be empty?

Go's `net` package [will unlink][1] for us, as long as we used Listen & friends to create the Unix socket. Go will even skip the unlink when the socket appears to be abstract (starts with a NUL, represented by an @), though we must be cautious to only create sockets with an abstract address on platforms that actually support it -- this caused [several][2] [bugs][3] before. [1]: https://pkg.go.dev/net#UnixListener.SetUnlinkOnClose [2]: docker/cli#4783 [3]: docker/cli#4863 Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

glours requested a review from silvin-lubecki as a code owner February 8, 2024 17:06

glours force-pushed the use-compose-go branch from f61e9a4 to b2467b2 Compare February 8, 2024 19:48

glours force-pushed the use-compose-go branch 5 times, most recently from 9d6bf37 to a696986 Compare February 8, 2024 20:47

ndeloof mentioned this pull request Feb 9, 2024

Adopt compose-specification for compose file support in docker stack command #2527

Open

glours force-pushed the use-compose-go branch 2 times, most recently from 0d2f012 to 6abad62 Compare February 9, 2024 09:09

ndeloof mentioned this pull request Feb 9, 2024

introduce publish (alpha) command docker/compose#10949

Merged

glours force-pushed the use-compose-go branch 2 times, most recently from 21487c5 to aa63cb7 Compare February 16, 2024 14:35

use compose-go to load and parse compose files

9705db2

Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>

glours force-pushed the use-compose-go branch from aa63cb7 to 9705db2 Compare February 16, 2024 14:43

neersighted mentioned this pull request Mar 22, 2024

plugin: drop explicit unlink #4963

Merged

Benehiko reviewed Mar 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use compose-go to load and parse compose files #4863

use compose-go to load and parse compose files #4863

glours commented Feb 8, 2024 •

edited

codecov-commenter commented Feb 8, 2024 •

edited

Radiergummi commented Feb 12, 2024

Leopere commented Feb 14, 2024

Benehiko left a comment

Benehiko Mar 25, 2024

Benehiko Mar 25, 2024

Benehiko Mar 25, 2024

Benehiko Mar 25, 2024

Benehiko Mar 25, 2024

Benehiko Mar 25, 2024


		specLoader "github.com/compose-spec/compose-go/v2/loader"
		compose "github.com/compose-spec/compose-go/v2/types"

use compose-go to load and parse compose files #4863

Are you sure you want to change the base?

use compose-go to load and parse compose files #4863

Conversation

glours commented Feb 8, 2024 • edited

codecov-commenter commented Feb 8, 2024 • edited

Codecov Report

Radiergummi commented Feb 12, 2024

Leopere commented Feb 14, 2024

Benehiko left a comment

Choose a reason for hiding this comment

Benehiko Mar 25, 2024

Choose a reason for hiding this comment

Benehiko Mar 25, 2024

Choose a reason for hiding this comment

Benehiko Mar 25, 2024

Choose a reason for hiding this comment

Benehiko Mar 25, 2024

Choose a reason for hiding this comment

Benehiko Mar 25, 2024

Choose a reason for hiding this comment

Benehiko Mar 25, 2024

Choose a reason for hiding this comment

glours commented Feb 8, 2024 •

edited

codecov-commenter commented Feb 8, 2024 •

edited