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

minor: plumb context through to tlog requests #1103

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

dekkagaijin
Copy link
Member

This makes the timeout argument unnecessary

NONE

@dekkagaijin dekkagaijin changed the title plumb context through to tlog requests minor: plumb context through to tlog requests Nov 24, 2021
@@ -67,6 +67,12 @@ func AttestCmd(ctx context.Context, ko sign.KeyOpts, regOpts options.RegistryOpt
return errors.Wrap(err, "parsing reference")
}

if timeout != time.Duration(0) {
Copy link
Member

Choose a reason for hiding this comment

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

This reads a lot better

Suggested change
if timeout != time.Duration(0) {
if timeout != 0 {

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -135,6 +134,13 @@ func SignCmd(ctx context.Context, ko KeyOpts, regOpts options.RegistryOptions, a
}
}

// TODO - Defaulting the timeout to zero as the CLI doesn't accept timeout.
Copy link
Member

Choose a reason for hiding this comment

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

IDK how to interpret the work needed to address this TODO

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed it up

@dekkagaijin dekkagaijin merged commit 6fc942b into sigstore:main Nov 24, 2021
@dekkagaijin dekkagaijin deleted the tlog-ctx branch November 24, 2021 22:41
@github-actions github-actions bot added this to the v1.4.0 milestone Nov 24, 2021
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.

2 participants