Skip to content

validation: create: don't skip errors on state#626

Merged
liangchenye merged 1 commit intoopencontainers:masterfrom
kinvolk-archives:alban/create
May 3, 2018
Merged

validation: create: don't skip errors on state#626
liangchenye merged 1 commit intoopencontainers:masterfrom
kinvolk-archives:alban/create

Conversation

@alban
Copy link
Copy Markdown
Contributor

@alban alban commented Apr 18, 2018

If the runtime fails to give us the state, don't just ignore this and
skip the test but ensure we print "not ok" in the TAP output.

Signed-off-by: Alban Crequy alban@kinvolk.io

If the runtime fails to give us the state, don't just ignore this and
skip the test but ensure we print "not ok" in the TAP output.

Signed-off-by: Alban Crequy <alban@kinvolk.io>
"state ID": state.ID,
})
} else {
t.Skip(1, "'state' MUST return the state of a container")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This effectively rolls back part of d8d2396 (#564), but I didn't motivate the skip then, and can't remember why I'd felt it was a good idea. I'm fine returning to to stricter handling like you've done here.


if err == nil {
state, err := r.State()
t.Ok(err == nil && state.ID == c.id, "'state' MUST return the state of a container")
Copy link
Copy Markdown
Contributor

@wking wking Apr 18, 2018

Choose a reason for hiding this comment

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

While we're touching this line, we may want to use the appropriate spec error. In this case, a failed state call would be violating the combination of EntityOperSameContainer, QueryStateImplement, and, from the CLI spec, “The runtime MUST support the entire API defined in this specification” and “Exit code: Zero if the state was successfully written to stdout and non-zero on errors” (which should really have a MUST, I'll file a PR for that). From among those, the most important seems like QueryStateImplement. Following this pattern would give us something like:

err = specerror.NewError(specerror.QueryStateImplement, errors.New("'state' returns the state of the container"), g.Config.Version)
runtimeError, ok := err.(*specerror.Error)
if !ok {
  return util.Fatal(fmt.Errorf("cannot convert %v to a runtime-spec error", err))
}
state, err := r.State()
t.Ok(err == nil && state.ID == c.id, runtimeError.Err.Err.Error())
diagnostic = map[string]string{
  "level": runtimeError.Err.Level.String(),
  "reference": runtimeError.Err.Reference,
  "container ID" = c.id,
}
if err == nil {
  diagnostic["state ID"] = state.ID
} else {
  diagnostic["error"] = err
  if e, ok := err.(*exec.ExitError); ok && len(e.Stderr) > 0 {
    diagnostic["stderr"] = string(e.Stderr)
  }
}
t.YAML(diagnostic)

Although we could make that a bit easier by adding a helper like func NewRFCError(code Code, err error, version string) rfc2119.Error to specerror. I've filed #627 to add NewRFCError.

@liangchenye
Copy link
Copy Markdown
Member

liangchenye commented May 3, 2018

LGTM

Approved with PullApprove

@liangchenye liangchenye merged commit ba17f13 into opencontainers:master May 3, 2018
@dongsupark dongsupark deleted the alban/create branch May 22, 2018 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants