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

File output flag for writing built images to a specified file #2476

Merged
merged 6 commits into from
Jul 16, 2019

Conversation

marlon-gamez
Copy link
Contributor

This is essentially the same as running skaffold build --quiet > file.out, but doesn't silence the standard output from skaffold.

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #2476 into master will increase coverage by 0.03%.
The diff coverage is 60%.

Impacted Files Coverage Δ
cmd/skaffold/app/cmd/build.go 79.41% <60%> (-9.05%) ⬇️
pkg/skaffold/docker/client.go 71.56% <0%> (+1.96%) ⬆️
pkg/skaffold/jib/jib.go 78.49% <0%> (+2.15%) ⬆️

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Jul 15, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 15, 2019
@tejal29
Copy link
Contributor

tejal29 commented Jul 15, 2019

This looks good! Few comments

  • if you have a --file-output flag on, the build output log lines should go to stdout. Is that happening? ( write tests to confirm that)
  • Write tests where
    -- both --quiet and --file-output is present. In this case, either you shd see build output on std out and also in the file or error out saying both are not supported. I feel 1st is better.
    -- both --file-output and -o are present the file has the correct formatted output.
    • also make sure, if the builder prints some output it is written to std out when --file-output is present and no --quiet.

cmdOut := flags.BuildOutput{Builds: bRes}

if buildOutputFlag != "" {
f, err := os.OpenFile(buildOutputFlag, os.O_RDWR|os.O_CREATE, 0755)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use ioutil.WriteFile like skaffold fix uses
here https://github.com/GoogleContainerTools/skaffold/blob/master/cmd/skaffold/app/cmd/fix.go#L73
if err := ioutil.WriteFile(configFile, newCfg, 0644); err != nil { }

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok.
Can you create a bytes.buffer and execute the template there

buildOut bytes.Buffer
t.Execute(buildOut, r)

And the if --quiet is present,

out.Write(buildOut)

if "--file-output" is present,

err := ioutil.WriteFile(buildOutputFlag, buildOut, 0644)
if err != nil {
  log.Fatal(err)
}

That ways its cleaner and we do not overwrite out

Copy link
Contributor

@priyawadhwa priyawadhwa 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 to me! could you add a unit test, maybe to TestQuietFlag or write a new one?

@priyawadhwa priyawadhwa added the kokoro:run runs the kokoro jobs on a PR label Jul 16, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 16, 2019
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

LGTM!

@priyawadhwa priyawadhwa merged commit c01388b into GoogleContainerTools:master Jul 16, 2019
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

yay!

@@ -104,6 +104,7 @@ Options:
--cache-file='': Specify the location of the cache file (default $HOME/.skaffold/cache)
-d, --default-repo='': Default repository value (overrides global config)
--enable-rpc=false: Enable gRPC for exposing Skaffold events (true by default for `skaffold dev`)
--file-output='': Filename to write build images to
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need "Used in conjunction with --quiet flag" for -o, --output? I guess from now on that flag even applies without quiet mode?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that should still be there, but be updated to say "Used in conjunction with --quiet or --file-output", since those are the only two flags whose output it effects.

@timowuttke
Copy link

Is there something off with the documentation? It works fine with skaffold build --file-output=blub but the documentation (and skaffold build --help) claims it should be skaffold build --output=blub which doesn't work.

@marlon-gamez
Copy link
Contributor Author

Is there something off with the documentation? It works fine with skaffold build --file-output=blub but the documentation (and skaffold build --help) claims it should be skaffold build --output=blub which doesn't work.

--output is actually the flag used to format the output of --quiet (and now --file-output as well). The naming is a bit confusing, sorry about that. Running skaffold build --help should show documentation for both --output and --file-output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants