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

introduce include to load sub-compose projects as dependencies #416

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Jun 6, 2023

This adds support for include attribute introduced in the compose-specification by compose-spec/compose-spec#363

This supports including compose sub-projects, comparable to language dependency management

usage:

include:
   - ../commons/compose.yaml
   - ../another-project/compose.yaml

services:
   foo:
      depends_on:
         - imported-service

required projects are resolved based on their own project directory to avoid relative path hell.

@ndeloof ndeloof force-pushed the require branch 6 times, most recently from 2cdf161 to f66a32e Compare June 7, 2023 17:54
@laurazard laurazard changed the title introduce require to load sub-compose projects as dependencies introduce include to load sub-compose projects as dependencies Jun 26, 2023
@laurazard laurazard changed the title introduce include to load sub-compose projects as dependencies introduce include to load sub-compose projects as dependencies Jun 26, 2023
@milas milas requested review from glours and milas June 26, 2023 22:57
@milas milas assigned milas and ndeloof and unassigned milas Jun 26, 2023
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

cli/options_test.go Outdated Show resolved Hide resolved
loader/include.go Outdated Show resolved Hide resolved
loader/include.go Outdated Show resolved Hide resolved
@ndeloof ndeloof force-pushed the require branch 2 times, most recently from 1064c01 to bad8119 Compare June 27, 2023 08:30
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

Sounds good to me, you just need to fix the conflict before merging

return requires, err
}

var transformIncludeConfig TransformerFunc = func(data interface{}) (interface{}, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can't we use generics here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this signature isn't a "lack of generics": those manage the parsed yaml nodes, which are actual interface{}

Comment on lines +307 to 317
err := ResolveRelativePaths(project)
if err != nil {
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could also be apply to the following if !opts. blocks

Suggested change
err := ResolveRelativePaths(project)
if err != nil {
return nil, err
}
if err := ResolveRelativePaths(project); err != nil {
return nil, err
}

Copy link
Collaborator Author

@ndeloof ndeloof Jul 3, 2023

Choose a reason for hiding this comment

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

I personally consider those to have lower readability for the tiny benefit of a single line being saved

especially as the "long" syntax is displayed in my goland IDE as a single "error handling" line:

        err := ResolveRelativePaths(project)
		if err != nil : nil, err

Copy link
Collaborator

Choose a reason for hiding this comment

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

As you wish, I don't have a strong opinion about this 😉


services:
foo:
image: foo
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: new line

@ndeloof ndeloof force-pushed the require branch 3 times, most recently from 21a9ed8 to aae3bba Compare July 3, 2023 13:16
@glours glours merged commit b3d5c66 into compose-spec:master Jul 4, 2023
@vasilvestre
Copy link

Could it work with URL later ?

@ndeloof
Copy link
Collaborator Author

ndeloof commented Jul 12, 2023

@vasilvestre this is indeed something we consider for future, also see compose-spec/compose-spec#288

@ndeloof ndeloof deleted the require branch July 12, 2023 07:28
@VladaPetrovic
Copy link

The functionality to include the remote file would be a great addition to this feature 🚀
It was already proposed in the original idea

for example:

include:
https://localhost:8000/some-service/compose.yaml
...

We could leverage this feature to re-use Compose files across teams. Currently, we’re cloning an additional repository solely to include another Compose file, which feels a bit cumbersome. 🤔

The mentioned compose-spec/compose-spec#288 seems like a broader feature with a much larger scope. While it could also address this problem, it approaches it from a different perspective.

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.

5 participants