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

feat: add compose run command #1340

Merged
merged 5 commits into from
Aug 31, 2022
Merged

feat: add compose run command #1340

merged 5 commits into from
Aug 31, 2022

Conversation

minuk-dev
Copy link
Contributor

@minuk-dev minuk-dev commented Aug 25, 2022

Signed-off-by: Min Uk Lee minuk.dev@gmail.com

close #270

Changes (for more details, see each item below):

  • remove -d from default RunArgs of serviceparser.Container
  • add apt install expect into docker-compatibility test
  • add Base.ComposeCmdWithHelper() into testutil
  • add compose run command (no details below)
  • add compose run docs into README.md (no details below)

Unimplmented Flags:

  • --no-TTY, --use-aliases

Not tested Flags:
- volumes, entrypoint
(I'll add additional test commit as soon as possible. I'm trying to figure out how to test it properly.) (Add test cases)

Details

remove -d from default RunArgs of serviceparser.Container

  • Previously, composer.upServiceContinaer() uses -d flag to find cid.

-d, --detach Run container in background and print container ID

  • Because of it, cannot support --interactive flag of compose run and StdinOpen option of compose-spec
  • Remove -d from default RunArgs and explicitly add property of container. Containers are now possible to determine whether or not to detach when they really start to run.
  • Alternatively, use --cid-file flag to find it.

add apt install expect into docker-compatibility test

add Base.ComposeCmdWithHelper() into testutil

  • As I mentioned, unbuffer needs to emulate tty to test compose run.
  • Basically, I referenced Base.CmdWithHelper()

Signed-off-by: Min Uk Lee <minuk.dev@gmail.com>
@minuk-dev

This comment was marked as resolved.

@minuk-dev
Copy link
Contributor Author

I added some test cases for --entrypoint, --volume.
Could anyone review, please?

@junnplus junnplus self-requested a review August 26, 2022 16:24
@AkihiroSuda AkihiroSuda self-requested a review August 28, 2022 19:06
@AkihiroSuda AkihiroSuda added this to the v0.22.3 milestone Aug 30, 2022
@fahedouch fahedouch self-requested a review August 30, 2022 14:42
pkg/composer/up_service.go Show resolved Hide resolved
pkg/composer/up_service.go Outdated Show resolved Hide resolved
pkg/composer/up_service.go Outdated Show resolved Hide resolved
pkg/composer/run.go Outdated Show resolved Hide resolved
minuk-dev and others added 4 commits August 31, 2022 07:04
Signed-off-by: Min Uk Lee <minuk.dev@gmail.com>
Signed-off-by: Min Uk Lee <minuk.dev@gmail.com>
Signed-off-by: Min Uk Lee <minuk.dev@gmail.com>

rewrite testcode

test commit
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry for delay in reviewing

@AkihiroSuda
Copy link
Member

Found a minor incompatibility: docker compose run randomize container name (compose-wordpress_db_run_cca27158caa2 ) but nerdctl compose run does not (`compose-wordpress_db_1).

@fahedouch
Copy link
Member

LGTM

@fahedouch
Copy link
Member

@AkihiroSuda could you plz ask for squashing commits before merging the PR in the future :) !

@AkihiroSuda
Copy link
Member

@AkihiroSuda could you plz ask for squashing commits before merging the PR in the future :) !

In this case having separate commits is fine, as each of the commits has a separate topic.

Add tests and docs for compose run command could be meld into Add compose run command template though.

@minuk-dev minuk-dev deleted the feat/compose-run branch August 31, 2022 09:55
@djdongjin djdongjin mentioned this pull request Nov 17, 2022
6 tasks
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.

Support Compose Run
3 participants