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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: rethrow original error #223

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

MilosRasic
Copy link
Contributor

Just suggesting this change because what's thrown right now isn't very useful for figuring out what went wrong:
image

But if we just re-throw the original error:
image

Now I know someone actually has a GCP project called "baby" but Secret Manager isn't enabled for it 馃檪

Tests are passing because they assert only that an error is thrown.

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

Well I guess in this case there's no point in catching the error in the first place then

@MilosRasic
Copy link
Contributor Author

Well I guess in this case there's no point in catching the error in the first place then

True. Removed try..catch. Feels good 馃檪

@simoneb
Copy link
Member

simoneb commented Aug 2, 2023

You may have to update some tests, CI is red

@MilosRasic
Copy link
Contributor Author

Looks like it's not even reaching the tests. Fails to authenticate with GCP
image

PRs triggered from forks don't get the secrets injected. If you don't mind potentially exposing the secrets, pull_request_target trigger instead of pull_request should work.

Otherwise, there are more complex options: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

@simoneb
Copy link
Member

simoneb commented Aug 2, 2023

Ah no you're right, I assumed the issue was different

@simoneb simoneb merged commit f81a53d into nearform:master Aug 2, 2023
1 of 5 checks passed
@github-actions github-actions bot mentioned this pull request Aug 3, 2023
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.

None yet

2 participants