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 top-level name #206

Merged
merged 4 commits into from Jan 22, 2022
Merged

introduce top-level name #206

merged 4 commits into from Jan 22, 2022

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Oct 2, 2021

What this PR does / why we need it:
introduce top-level name to address docker/compose#745

@thaJeztah
Copy link
Member

No, I don't think we should do this, and this definitely doesn't address docker/compose#745 (just shifts the problem from directory name or .env to the compose file)

@ndeloof
Copy link
Collaborator Author

ndeloof commented Oct 2, 2021

@thaJeztah then I misunderstood the requirements set on docker/compose#745

@ozydingo
Copy link

ozydingo commented Oct 2, 2021

@thaJeztah do you mean because it doesn't persist the project name in a separate file from the docker-compose.yml file, and/or because compose does not save the auto-generated project name into said separate file? This solution does allow persistence of the project name independently of choices that may affect other (not compose related) parts of a project (folder name, checking in a .env file), so I think it's fair to say it covers at least some of the use cases -- I think some clarification on what it's missing would be helpful.

@ndeloof
Copy link
Collaborator Author

ndeloof commented Oct 3, 2021

@thaJeztah ok, reading more carefully your initial request docker/compose#745.

IIUC you're expecting we store "user session state" on filesystem aside the compose.yaml file so that user only has to set --project-name once and then all further commands will apply to the same project. We could obviously have docker-compose support some .docker-compose file in project directory as an implementation specific alternative to define project name, as well as some additional command flag to create this file. That could be nice (I'm not fully convince this is really useful, but why not ¯\(ツ)/¯ ?

That being said, comments on docker/compose#745 demonstrate we also have user who expect they can hard-code the project name in the compose file to make it fully deterministic. I think we can have both

  • default project name defined in compose file
  • if not set, default project name determined by implementation (docker-compose uses parent folder)
  • project name set by some implementation-specific config file
  • project name set explicitly by command

@gtardif
Copy link
Collaborator

gtardif commented Oct 3, 2021

+1 to have an option to define name as part of the compose file. Indeed, it does not need to be the only option, but in many cases defining it inside the compose file is the most intiuitive and natural solution.
Then, ability to store project name passed by --project-name or other ways when compose up can be used to override the name passed in the compose file (that in turn also overrides default naming like based on parent folder).
Note that compose implementation also uses container labels to store project name, not sure about the interaction with this - but adding the name field in the compose file can be done quite independently of more "runtime" features happening related to storing project name after user executes compose up.

Copy link
Collaborator

@EricHripko EricHripko left a comment

Choose a reason for hiding this comment

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

Tiny typo inline 💬 More general question: how do we expect it to work for cases when multiple Compose files are combined?

spec.md Outdated Show resolved Hide resolved
@ndeloof
Copy link
Collaborator Author

ndeloof commented Nov 19, 2021

how do we expect it to work for cases when multiple Compose files are combined

Like other elements : if defined in an override file, will override.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Collaborator

@EricHripko EricHripko left a comment

Choose a reason for hiding this comment

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

Looks good (with one typo/suggestion inline) ✅

spec.md Outdated Show resolved Hide resolved
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Collaborator

@EricHripko EricHripko left a comment

Choose a reason for hiding this comment

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

Looks great ✅ Very excited about this addition 🎉

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.

None yet

6 participants