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 tty prompt error for keypair generation #688

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Commits on Jun 13, 2022

  1. Improve tty prompt error for keypair generation

    When a prompt is used, it may happen that /dev/tty is not available.
    In many cases it's not clear to the user what exactly went wrong in
    such a case, because that context is lost. Using STEPDEBUG=1 may
    help in these cases, but the user must know about that. Arguably,
    the stacktrace in the output isn't very helpful for casual users.
    
    In this commit we opt for an explicit check when a prompt is requested
    to see if the error indicates that `/dev/tty` is unavailable. The error
    can be contextualized by the caller. This is a proposal and hasn't been
    applied to all cases of a prompt potentially failing, because Joe and
    I first wanted some feedback on this approach.
    
    I think it would be nice if we could take this even further, like
    including a `context.Context` in the call to the prompt, so that
    additional information could be attached and potentially used when
    creating or printing the error. Right now it's up to all callers to
    handle this specific case, because only the callers know the context
    of the call and what the user or system was trying to do with the
    call. It's not trivial to devise a (more) elegant way to provide
    this context to the password prompt without `context.Context`. The
    alternative is to ensure we always wrap errors with the function
    that called them, so that the (full) lineage of the error can be
    seen. `errors.As` and `errors.Is` will still work with that approach.
    The current version of `urfave/cli` doesn't play super nice with
    `context.Context`; v2 seems to do a better job at that, but requires
    a bigger refactor.
    
    In this commit we don't make use of the `errors.Wrap` functionality,
    which results in `STEPDEBUG=1` not printing the stacktrace for an
    error. This may be acceptable if the caller provides the context, but
    we could add it back in if we want to or find a different solution.
    
    The generalized utility function can probably be moved to
    `smallstep/cli-utils` at a later stage.
    
    Relates to #674
    hslatman committed Jun 13, 2022
    Configuration menu
    Copy the full SHA
    e80377e View commit details
    Browse the repository at this point in the history