feat: validate proposed change(s) in triagebot.toml for PRs ✨

meysam81 commented 9 months ago

fixes rust-lang/rust#106104

A successful run is visible here: https://github.com/meysam81/triagebot-test/pull/2#issuecomment-1751986969

Some highlights I need to mention here:

I can't be sure if the proposed change is placed in the right spot, but it works! :shrug:
If it's not too much to ask, would you please add hacktoberfest-accepted to this PR, since the repo is not marked with hacktoberfest.
One question of mine is: why is cargo fmt, nor cargo clippy not formatting my code? :thinking:
How can I write test for this? I couldn't figure this part out!

meysam81 commented 9 months ago

r?

rustbot commented 9 months ago

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

meysam81 commented 9 months ago

r? dev-tools

meysam81 commented 8 months ago

@Mark-Simulacrum

As you suggested, I refactored the changes into its own module.

You may also see a successful run here: https://github.com/meysam81/triagebot-test/pull/2#issuecomment-1762596593

ehuss commented 8 months ago

BTW, there is now a merge conflict.

ehuss commented 7 months ago

I'm a bit confused about the approach taken here. It seems to validate the config on every PR opened or pushed. I would expect it to operate like it did before, where it would only validate the config on a PR that is modifying triagebot.toml. Can you say more about why that is changing? I would also not expect there to be any need to do any caching if it was only checking in the case where triagebot.toml is added or modified.

meysam81 commented 7 months ago

I'm a bit confused about the approach taken here. It seems to validate the config on every PR opened or pushed. I would expect it to operate like it did before, where it would only validate the config on a PR that is modifying triagebot.toml. Can you say more about why that is changing? I would also not expect there to be any need to do any caching if it was only checking in the case where triagebot.toml is added or modified.

The main driver for the new approach was to address the expensive HTTP round-trips mentioned here: https://github.com/rust-lang/triagebot/pull/1730#discussion_r1349717753

Additionally, at the expense of using a fixed size memory for holding the config (128 total triagebot.toml ATM), we achieve a speed up in case no new commits has been pushed and only a synchronization is triggered.

ehuss commented 7 months ago

The main driver for the new approach was to address the expensive HTTP round-trips mentioned here: https://github.com/rust-lang/triagebot/pull/1730#discussion_r1349717753

My concern in that comment was about using the diff endpoint which fetches the entire contents of every PR. Would it be possible to use the files() endpoint instead to check if the PR touches triagebot.toml, and only download the file in that case? I don't think that approach should need any sort of caching.

In the future, we could potentially cache the files() output, since I think there are several other handlers that need to check if a PR touches certain files. But that cache would only need to live as long as the loop through the handlers, and then can be immediately discarded. But I think that is something we could consider later, not something I would bother with in this PR.

ehuss commented 7 months ago

Hm, so I'm taking a closer look at this, and I need the diff for something else. We're already fetching the diff 3 times in other handlers. I'll try to follow up soon with something that caches that.

ehuss commented 7 months ago

Ok, I posted #1749 to implement caching of the diff (since I need it for something else). If/when that PR is merged, I think this PR can be updated to just call the diff() function to determine if it contains triagebot.toml, and if it does, fetch the contents and validate it (much like it did before 287b41499331d1828055acdd6ba5fcd00fff92b3).

Sorry about the back and forth on this.

ehuss commented 5 months ago

I hope you don't mind, I went ahead and applied some of the changes suggested above. I also made a few other changes:

Updated toml to the latest version to get better error messages.
Added deny_unknown_fields to show typo errors to the user.
Switch to using SHA for the file download instead of the git ref. The git ref has issues with caching, and can take several minutes to update. This also uses the pre-existing code for fetching raw files.

Thanks for helping with this!

apiraino commented 5 months ago

Added deny_unknown_fields to show typo errors to the user.

@ehuss IIUC I think commit 252acb2a1f43caa67b972192a467eb97632764fa caused unexpected side-effects (see comment).

It will report errors also on PRs adding new config options to triagebot.

rust-lang / triagebot

feat: validate proposed change(s) in triagebot.toml for PRs ✨ #1730