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

Ignoring errors is a bad idea #27

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

andrewjhumphrey
Copy link
Contributor

This error didn't surface very often as it required the GetRole call to fail.
The "normal" reason a GetRole fails is permission denied, which doesn't much happen in our context as folks run iamy using a privileged role.
Less common are network faults, like the one Bing experienced. Ignoring the error lead to an attempt to use memory that hadn't been allocated, with predictable consequences.

I've chosen to just panic if the role can't be retrieved as it means we don't have any information about that role and we don't want to save to disk or sync an incomplete state. We could theoretically add retry and backoff logic to the GetRole call and see this error occur less, but in the case of a PermissionDenied it doesn't matter how many times we retry, we're never going to get the information we need.

@andrewjhumphrey andrewjhumphrey merged commit e76d232 into main Jun 20, 2022
@andrewjhumphrey andrewjhumphrey deleted the andrewjhumphrey-do-not-ignore-errors branch June 20, 2022 05:14
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.

4 participants