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

support --compose-file - as stdin #347

Merged
merged 1 commit into from Aug 22, 2017

Conversation

@mmariani
Copy link
Contributor

commented Jul 18, 2017

As discussed in moby/moby#34036

@mmariani mmariani force-pushed the Wolphin-project:stdin branch from 3461152 to 708edf5 Jul 18, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jul 18, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Jul 18, 2017

Codecov Report

Merging #347 into master will increase coverage by 0.01%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   47.02%   47.04%   +0.01%     
==========================================
  Files         198      198              
  Lines       16336    16349      +13     
==========================================
+ Hits         7682     7691       +9     
- Misses       8260     8263       +3     
- Partials      394      395       +1
@mmariani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

I would have added a test but I'm pretty new at go and don't see a clean way to mock /dev/stdin.

@dnephin
Copy link
Collaborator

left a comment

Thanks! this is a good start

@@ -57,6 +57,8 @@ Creating service vossibility_ghollector
Creating service vossibility_lookupd
```

The Compose file can also be provided as standard input with `--compose-file -`.

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 18, 2017

Collaborator

I think this would be better in the help text for the --compose-file flag. Something like

Path to a Compose file, use "-" to read from stdin

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Jul 24, 2017

Contributor

I agree, then it doesn't need to be specifically dealt with in the reference file at all, unless you wanted to add an example (which would probably be good to have).

if err != nil {
return details, err
if composefile == "-" && runtime.GOOS != "windows" {
composefile = "/dev/stdin"

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 18, 2017

Collaborator

I think we can make this easier to test, and supported on windows.

Instead of setting composefile = "/dev/stdin" here, only set WorkingDir.

You'll also need to pass dockerCli.In() as an io.Reader from deployCompose() where this is called, so we have access to it in getConfigFile().

In getConfigFile before calling ioutil.ReadFile() check if filename == "-" and use this instead to get the bytes:

bytes, err = ioutil.ReadAll(stdin)

@mmariani mmariani force-pushed the Wolphin-project:stdin branch from 708edf5 to 0872028 Jul 24, 2017

@mmariani mmariani requested a review from vdemeester as a code owner Jul 24, 2017

@mmariani mmariani force-pushed the Wolphin-project:stdin branch 2 times, most recently from dcc15b8 to 72662e6 Jul 24, 2017

@mmariani mmariani requested review from mistyhacks and thaJeztah as code owners Jul 24, 2017

@mmariani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

this one should do, thanks

@@ -150,15 +159,24 @@ func buildEnvironment(env []string) (map[string]string, error) {
return result, nil
}

func getConfigFile(filename string) (*composetypes.ConfigFile, error) {
bytes, err := ioutil.ReadFile(filename)
func getConfigFile(filename string, stdin io.Reader) (*composetypes.ConfigFile, error) {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 24, 2017

Member

Wondering if this should just take an io.Reader in both cases @dnephin wdyt?

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 26, 2017

Collaborator

Ya, that sounds like a good idea

This comment has been minimized.

Copy link
@mmariani

mmariani Jul 27, 2017

Author Contributor

I did not do that, because it needs the filename in the return.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 14, 2017

Member

Ah, I see; yes, we may have to refactor some things. None of these are exported functions, so I guess we can do that in a follow up.

@@ -57,6 +57,8 @@ Creating service vossibility_ghollector
Creating service vossibility_lookupd
```

The Compose file can also be provided as standard input with `--compose-file -`.

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Jul 24, 2017

Contributor

I agree, then it doesn't need to be specifically dealt with in the reference file at all, unless you wanted to add an example (which would probably be good to have).

@docker docker deleted a comment from GordonTheTurtle Aug 1, 2017

@thaJeztah
Copy link
Member

left a comment

one nit, but LGTM otherwise; can you squash your commits? I think it's ok to keep the docs and code changes in the same commit

version: "3.0"
services:
foo:
image: alpine:3.5

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 14, 2017

Member

Looks like the indentation got lost here. We're not testing if the content is actually parsed, so won't make a difference (currently); I see the other test also doesn't do this, but wondering if we should, e.g.;

diff --git a/cli/command/stack/deploy_composefile_test.go b/cli/command/stack/deploy_composefile_test.go
index 90d866a2..6e2a1f2c 100644
--- a/cli/command/stack/deploy_composefile_test.go
+++ b/cli/command/stack/deploy_composefile_test.go
@@ -37,15 +37,16 @@ func TestGetConfigDetailsStdin(t *testing.T) {
        content := `
 version: "3.0"
 services:
-foo:
-image: alpine:3.5
+  foo:
+    image: alpine:3.5
 `
        details, err := getConfigDetails("-", strings.NewReader(content))
        require.NoError(t, err)
        cwd, err := os.Getwd()
        require.NoError(t, err)
        assert.Equal(t, cwd, details.WorkingDir)
-       assert.Len(t, details.ConfigFiles, 1)
+       require.Len(t, details.ConfigFiles, 1)
+       assert.Equal(t, "3.0", details.ConfigFiles[0].Config["version"])
        assert.Len(t, details.Environment, len(os.Environ()))
 }
@vdemeester
Copy link
Member

left a comment

SGTM
@mmariani can you take care of @thaJeztah's comment (and squash your commits) ? 👼

@mmariani mmariani force-pushed the Wolphin-project:stdin branch 3 times, most recently from c86c765 to ac16f26 Aug 22, 2017

support --compose-file - as stdin
Signed-off-by: Marco Mariani <marco.mariani@alterway.fr>

@mmariani mmariani force-pushed the Wolphin-project:stdin branch from ac16f26 to 3a0b967 Aug 22, 2017

@mmariani

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2017

done, thanks for reminding. also updated to master

@dnephin
Copy link
Collaborator

left a comment

LGTM

@dnephin dnephin merged commit 0d17ea2 into docker:master Aug 22, 2017

8 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 70.83% of diff hit (target 50%)
Details
codecov/project 47.04% (+0.01%) compared to 317b735
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.