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

do not hard fail on failure to parse codeowners file #83

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

dfrankland
Copy link
Member

codeowners uses unwrap, so we need to fork the library to allow us to handle errors. If we cannot parse a codeowners file a debug log will be emitted:

2024-08-14T16:43:26 [DEBUG] - Found CODEOWNERS file `/Users/dylan/GitHub/flake-factory/./CODEOWNERS`, but couldn't parse it.

Copy link
Member

@mmatheson mmatheson 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 not a crab, but this looks good to me

Copy link

trunk-staging-io bot commented Aug 15, 2024

✅ 21 passed ⋅ (learn more)

settingsdocs ⋅ learn more about trunk.io

return Some(owners);
}
log::debug!(
"Found CODEOWNERS file `{}`, but couldn't parse it.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be error!?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, let's log the parsing error.

Copy link
Member Author

@dfrankland dfrankland Aug 15, 2024

Choose a reason for hiding this comment

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

I'm fine with that. Do we want to show this error to the users though? We plan to follow up by uploading the CODEOWNERS file regardless so that we can eventually post-process

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the parsing error isn't useful—it's just a PatternError from the glob crate. That won't help anyone understand why their file isn't correct.

@zaycev
Copy link
Contributor

zaycev commented Aug 15, 2024

You may need to update workflows to use new paths. Specifically .github/workflows/release.yml, but might be others as well.

@dfrankland
Copy link
Member Author

I'll double-check the release.yml workflow, but it should work fine. cargo will intelligently build and run things

@dfrankland dfrankland force-pushed the dylan/parse-codeowners-soft-error branch from 65385a2 to c0aa112 Compare August 15, 2024 15:00
@dfrankland
Copy link
Member Author

I updated the log to look like this:

2024-08-15T07:55:02 [ERROR] - Found CODEOWNERS file `/Users/dylan/GitHub/flake-factory/./CODEOWNERS`, but couldn't parse it: Pattern syntax error near position 3: invalid range pattern

Also, I updated the code to use file.is_file() rather than file.exists()... Since macOS fs is case-insensitive, our test run on CI would hang indefinitely since the CLI would find the new codeowners directory and try to open it as a file 🤦

@dfrankland dfrankland merged commit bc912a1 into main Aug 15, 2024
8 checks passed
@dfrankland dfrankland deleted the dylan/parse-codeowners-soft-error branch August 15, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants