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

(enhancement): Surface inner exception when secret:bulk upload command fails #227

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

AdiRishi
Copy link
Contributor

@AdiRishi AdiRishi commented Jan 3, 2024

Resolves #226

@AdiRishi AdiRishi requested a review from a team as a code owner January 3, 2024 02:13
@petebacondarwin
Copy link
Contributor

Does this actually resolve the error surfacing? I have a feeling that Wrangler is swallowing the error before it even gets to the wrangler-action.

@AdiRishi
Copy link
Contributor Author

AdiRishi commented Jan 3, 2024

Does this actually resolve the error surfacing? I have a feeling that Wrangler is swallowing the error before it even gets to the wrangler-action.

You're right in the sense that if wrangler cli itself is swallowing the error, this won't help.
However if that's not the case, at the very least this will log what we get back from the CLI output. Right now, even that error is swallowed and we just throw a generic Failed to upload secrets. error.

@1000hz
Copy link
Contributor

1000hz commented Jan 3, 2024

I think in this case the error will be swallowed by @actions/exec, which will throw an error like The process npx failed with exit code 1. We need to add listeners to capture output from the running command as described here:
https://github.com/actions/toolkit/blob/main/packages/exec/README.md#outputoptions

@AdiRishi
Copy link
Contributor Author

AdiRishi commented Jan 3, 2024

I think in this case the error will be swallowed by @actions/exec, which will throw an error like The process npx failed with exit code 1. We need to add listeners to capture output from the running command as described here: https://github.com/actions/toolkit/blob/main/packages/exec/README.md#outputoptions

Oh I see! Thank you, was not aware of this. I'll update the PR

@1000hz
Copy link
Contributor

1000hz commented Jan 4, 2024

Sorry, I got mixed up. Adding listeners to capture output from exec() shouldn't be necessary to pipe the results to stdout/stderr of the parent process, since that's already the default behavior. I suspect the error output is indeed being swallowed by wrangler itself.

Copy link
Contributor

@1000hz 1000hz left a comment

Choose a reason for hiding this comment

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

I'm a little suspicious that exec() isn't actually executing during some of the reports of failed secret uploads, so I think this PR will be helpful to narrow down what's causing things to fail in case it's indeed happening before we actually execute wrangler.

Please add a changeset by running npx changeset describing this change.

src/index.ts Outdated
} catch (err: unknown) {
if (err instanceof Error) {
error(err.message);
debug(`wrangler secret:bulk error stack: ${err.stack}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
debug(`wrangler secret:bulk error stack: ${err.stack}`);
debug(err.stack);

@AdiRishi
Copy link
Contributor Author

AdiRishi commented Jan 5, 2024

Thanks again for the CR, definitely learned a few things about exec and it's output. If you'd like I can replace exec with getExecOutput and we can add stdout/stderr to the debug log as well.

Copy link
Contributor

@1000hz 1000hz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@1000hz 1000hz merged commit 522c117 into cloudflare:main Jan 5, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Jan 5, 2024
@AdiRishi AdiRishi deleted the arishi-secrets-error branch January 5, 2024 22: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.

(enhancement): Surface inner exception when uploading secrets fails
3 participants