-
Notifications
You must be signed in to change notification settings - Fork 455
Add Compose #239
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
base: main
Are you sure you want to change the base?
Add Compose #239
Conversation
… and container logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed as much as I can!
I'm fairly limited due to my current MacOS version as anything below 6.2.0 will just error out.
Although, thanks for your work on this feature request as I was really hopeful for such a feature!
So on Good Faith, I'm going to Approve! If anyone else wants to review, feel free!
Thank you! I am also planning to add support for the newfound networking capabilities in the coming days. Also, if anyone knows how to restructure compose to call these commands without invoking the command line, I would be very grateful to hear about it! |
To address these things before the merge, I am going to make this pull request into a draft. If you want to try the tool compiled, you can get it at https://github.com/mcrich23/Container-Compose. |
It's simply what I explained above. Let's say I'm submit a contribution to a big open source project, say Linux, or an Apache Foundation or CNCF project, and you're a maintainer making decisions about all the contributions that come in, including mine. You need to make sure that anything that is contributed is maintainable and reliable over time, and not only by the original contributor who may or may not be engaged with the project in the future. The way to do that is to avoid large PRs like this one, and instead break epic features into small deliverable units just as one would in most any agile software development process. It's far easier to ensure that cross-cutting concerns (logging, error handling, security, and so on) are addressed properly with fewer files, and once those are addressed it establishes a positive pattern for the future commits. It makes everyone's job easier, especially in the case that others see your promising work and want to contribute as well. From there we add container, network, and storage features one item at a time, with README documentation so it's crystal clear what's currently supported and what isn't. I can't be sure that anything "works consistently" yet because there are 35 files in the PR and zero tests. Again, with a smaller commit, it's simpler to include unit tests for what can be unit tested (granted, much of what container does is about side effects, but there may be some code here that could benefit from unit coverage). I hope this explains why, but please follow up if I'm not communicating the need clearly! |
With a small initial PR, we can focusing on solving that rather large usability problem so that while it's a very limited feature set to start with, it's one that works very well. |
Thanks for that explanation @jglogan. I am pretty busy for the next month or two, but I can send a sort of "State of Compose" surrounding how key components work and what is supported if you or others want to start pulling my work apart into smaller PRs. Just let me know 😄. I tried to keep everything documented, which should help this effort. Also, I totally agree that tests are needed, I just have not had times to write them. Hopefully that will happen as this is broken down into smaller PRs. |
@Mcrich23 All good! You've got a great proof of concept in place. There's definitely interest in this feature so I hope we'll all be able to find a little time in coming weeks to take what's here and build out a really cool compose plugin. Better to have it move slowly and thoughtfully than create something too quickly that's difficult for users and a maintenance burden. |
I have noticed that some repos also do decision markdown files to talk about stuff. Is that something we want to do for compose? Also, would it be better to do this as a git submodule and maintain a separate issue and pr list for compose? |
The State of ComposeThis is just a brief overview of current support and how certain things work! cc: @jglogan @stevenanthonyrevo @niteshbalusu11
Full/Semi-Supported FeaturesNetworking
Environment
Depended By
Volume Linking
Images
Documented Unsupported Features
|
@Mcrich23 I have a similar branch with a complete implementation of "compose" with tests and everything. I will try to make this into a standalone plugin as suggested by @jglogan https://github.com/mazdak/container/tree/mazdak/container-compose p.s. how are you working around the permission issues with Postgres (not being allowed to chown/chmod)? |
@mazdak Sounds good! What do you mean about the issues with Postgres and chown? |
I am unable to get the standard Postgres or redis images to boot up because their entrypoint script tries to modify the mounted volume permissions. This is a limitation of VirtioFS if I understand correctly. You won't notice this issue if you are not using a persistent mounted volume. |
It could be cause i'm expressly setting the container to be owned by the user? But haven't encountered that error in my testing. Maybe take a look at my implementation |
|
Adding the feature for requesting a specific IP (#282) isn't terribly hard, it's just that DNS names work today (but requires configuration as you note) so it's not our top priority. |
Yeah. I guess the way that Vapor does it by default avoids this issue so I never encountered that specifically. |
This is a rough sketch of how one could create a standalone swift package that provides a reader using Compose types generated from the JSON schema specification (the one under compose-spec/compose-go seems to be the canonical copy). |
@jglogan I totally agree that the ideal way to do this is by autogenerated codable structures via quicktype. My wonder is if totally valid yaml that slightly differs from the mainstream definition will throw errors. One example is: depends_on: foo versus depends_on:
- foo |
This particular case doesn't seem totally valid. % docker compose -f docker-compose-syntax.yaml up -d
validating docker-compose-syntax.yaml: services.server.depends_on must be a list
% echo $?
1 docker-compose-syntax.yaml:
|
I apologize, that is a bad example, but it is true for other fields. |
No apologies needed! I'm just curious to see cases where the parser for Docker Compose would admit cases that don't comply to the JSON schema. This doc describes everything the Compose parser does to transform YAML text into bound types: https://github.com/compose-spec/compose-go/blob/main/parsing.md. |
@jglogan So sorry for the delay. A lot going on personally including preparing for college (whoohoo!). Anyway, this is one example that comes to mind. I feel like we maybe start with QuickType and find a way to fix these one off issues as we go. The good news is that people won't have an issue as long as people follow typical syntax instead of shortcuts. |
I wonder if we can make a general decoding pattern that allows all string arrays to be a single string outside of an array, etc. Maybe member attribute or accessory macros could auto fill it so that just gets prepended to structs and we can still use QuickType. |
* improve compose file not found error message * support compose.yaml and make usage consistent across up and down * make compose.yml the default in error message
There has been a lot of discussion around a docker-compose counterpart for Container. This moves that discussion a bit further by introducing a basic translation layer for compose.
This pr introduces a new command group into the cli:
Compose
. It has two subcommands:up
anddown
.The origin behind this pr being made comes from the desire to run Vapor on Apple Containers. Hopefully, efforts like this make it possible quite soon.