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

Improve error output when kompose fails #3299

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

dgageot
Copy link
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

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
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
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
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

@@ -74,7 +74,7 @@ func (*Commander) RunCmdOut(cmd *exec.Cmd) ([]byte, error) {

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
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
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
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