search

firecracker-microvm / firectl

firectl is a command-line tool to run Firecracker microVMs
Apache License 2.0
477 stars 72 forks source link

Add test for getFirecrackerConfig #9

Closed IRCody closed 5 years ago

IRCody commented 5 years ago

Description of changes: Add tests for getFirecrackerConfig() just as a quick sanity check that it is handling errors from other parsing functions correctly.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

IRCody commented 5 years ago

Ill look at using wrap+the causer interface as a clean way to retain the context for the user while not parsing error strings.

On Fri, Jan 4, 2019 at 12:28 PM Maksym Pavlenko notifications@github.com wrote:

@mxpv commented on this pull request.

In options.go https://github.com/firecracker-microvm/firectl/pull/9#discussion_r245411347 :

@@ -165,7 +170,7 @@ func (opts *options) handleFifos() (io.Writer, error) { }

  if fifo, err = createFifoFileLogs(opts.FcFifoLogFile); err != nil {
  • return nil, fmt.Errorf("failed to create fifo log file: %v", err)
  • return nil, unableToCreateFifoLogFile

I don't think it's a good idea to drop inner error, it might keep valuable information for user.

— You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub https://github.com/firecracker-microvm/firectl/pull/9#pullrequestreview-189516927, or mute the thread https://github.com/notifications/unsubscribe-auth/AEvUYOQqAXK-qCjAyV6HevhOHhDDBEcZks5u_7l5gaJpZM4Zsvn- .

IRCody commented 5 years ago

I want to squash these commits to 1 before merging but left them here to make it easier to see the changes from comments.