Skip to content

Improve error output when kompose fails#3299

Merged
balopat merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:improve-err-output
Nov 27, 2019
Merged

Improve error output when kompose fails#3299
balopat merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:improve-err-output

Conversation

@dgageot
Copy link
Copy Markdown
Contributor

@dgageot dgageot commented Nov 27, 2019

  • Improve the output of util.RunCmdOut() in case of error.
  • Use that improved output when kompose fails

Before:

$ skaffold init --compose-file docker-compose.yaml
FATA[0000] running kompose: exit status 1

After:

$ skaffold init --compose-file docker-compose.yaml
FATA[0000] Running [kompose convert -f docker-compose.yaml]
 - stdout:
 - stderr: FATA ima_ge Additional property ima_ge is not allowed
: exit status 1

Signed-off-by: David Gageot david@gageot.net

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2019

Codecov Report

Merging #3299 into master will increase coverage by 0.03%.
The diff coverage is 80%.

Impacted Files Coverage Δ
pkg/skaffold/util/cmd.go 48.38% <100%> (ø) ⬆️
pkg/skaffold/initializer/init.go 59.74% <77.77%> (+1.3%) ⬆️

+ Improve error when compose fails
+ Check if the compose-file exists
+ Add test

Fix GoogleContainerTools#3288

Signed-off-by: David Gageot <david@gageot.net>
// runKompose runs the `kompose` CLI before running skaffold init
func runKompose(ctx context.Context, composeFile string) error {
if _, err := os.Stat(composeFile); os.IsNotExist(err) {
return err
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does err include the filename? Should this be wrapped with unable to find composeFile? Or should we just continue down into kompose and let it show an error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It'll show the filename

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It'll show the filename as is

err = cmd.Wait()
if err != nil {
return stdout, errors.Wrapf(err, "Running %s: stdout %s, stderr: %s, err: %v", cmd.Args, stdout, stderr, err)
return stdout, errors.Wrapf(err, "Running %s\n - stdout: %s\n - stderr: %s", cmd.Args, stdout, stderr)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Embedding newlines doesn't seem great. It feels like this should be an object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could do that. And it would have an Error() function that embeds newlines...
I'd rather keep this simple form

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And that object would have an Error() function with that string... I'd rather keep this simple form

@balopat balopat merged commit 87abe53 into GoogleContainerTools:master Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants