feat: validate proposed change(s) in triagebot.toml for PRs ✨ by meysam81 · Pull Request #1730 · rust-lang/triagebot

meysam81 · 2023-10-08T10:29:05Z

fixes rust-lang/rust#106104

A successful run is visible here: meysam81/triagebot-test#2 (comment)

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! 🤷
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? 🤔
How can I write test for this? I couldn't figure this part out!

meysam81 · 2023-10-08T10:39:51Z

r?

rustbot · 2023-10-08T10:39:53Z

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 · 2023-10-08T10:41:21Z

r? dev-tools

apiraino · 2023-10-08T15:17:31Z

+pub fn config_file_name() -> &'static str {
+    CONFIG_FILE_NAME
+}
+


why this utility function? Can CONFIG_FILE_NAME be made public and used directly in handler.rs?

apiraino · 2023-10-08T15:27:42Z

    errors
 }

+async fn validate_triagebot(ctx: &Context, event: &IssuesEvent, errors: &mut Vec<HandlerError>) {


As far as I can see this function would inject an HTTP call in the issue_handlers macro which is called quite often. I am a bit worried about the additional overhead on every issue the triagebot is called to handle.

What is your goal here @meysam81 ? validate the triagebot config for a certain repository, right? Can this check happen just once on triagebot startup?

Also the name of this function could be a bit more explicit about its purpose (e.g. validate_triagebot_config or something similar).

This function should be placed in its own module at minimum (similar to all the other handlers). I'd emulate something like rustc_commits in terms of structure.

I would expect that if the goal is to validate PRs, then we would do this only on push events. It probably also makes sense to delegate the actual validation to the config module, not just calling toml::from_slice here.

meysam81 · 2023-10-14T05:04:40Z

@Mark-Simulacrum

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

You may also see a successful run here: meysam81/triagebot-test#2 (comment)

ehuss · 2023-10-17T15:39:47Z

+    let diff = match event.issue.diff(&ctx.github).await {
+        Ok(Some(diff)) => diff,
+        wildcard => {
+            log::error!("Failed to fetch diff: {:?}", wildcard);
+            return Ok(None);
+        }
+    };
+
+    if diff.contains(crate::config::CONFIG_FILE_NAME) {


I have a few concerns about this. IIUC, this fetches the full diff on every PR on every push. That seems like it would be expensive, and potentially slow.

Would it be possible to use the files() method instead? I would imagine that is more efficient, and is also structured.

Similarly, this looks like it does a string contains check, which means that if any text anywhere in the patch contains the bytes triagebot.toml, it would trigger. If it looked at the structured file list, then it should be able to look to see if that file is specifically being modified.

I'm still not sure if hitting the files endpoint on every push is OK, but I suspect the traffic should be low enough to be fine.

ehuss · 2023-10-17T15:40:01Z

BTW, there is now a merge conflict.

ehuss · 2023-11-13T20:55:14Z

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.

ehuss · 2023-11-13T20:56:53Z

+        return triagebot_toml.clone();
+    }
+
+    log::warn!(


I think doing a warn! on every cache miss would pollute the logs with unactionable information. I would expect something like this to be trace or debug. Similarly with the info! above about a hit, that information is likely to add noise.

meysam81 · 2023-11-14T03:29:21Z

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: #1730 (comment)

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.

apiraino · 2023-11-14T16:30:58Z

    pub sha: String,
    pub filename: String,
    pub blob_url: String,
+    pub raw_url: String,


Are raw_url and Clone still needed?

ehuss · 2023-11-16T04:11:18Z

The main driver for the new approach was to address the expensive HTTP round-trips mentioned here: #1730 (comment)

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 · 2023-11-16T21:59:28Z

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 · 2023-11-17T04:59:48Z

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 287b414).

Sorry about the back and forth on this.

This adds deny_unknown_fields to Config so that any typo mistakes will be caught by the config validation.

fixes rust-lang/rust#106104

ehuss · 2024-01-22T00:49:24Z

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 · 2024-01-24T11:08:26Z

Added deny_unknown_fields to show typo errors to the user.

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

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

meysam81 mentioned this pull request Oct 8, 2023

Validate triagebot.toml in tidy/CI rust-lang/rust#106104

Closed

apiraino reviewed Oct 8, 2023

View reviewed changes

meysam81 changed the title ~~feat: valdiate proposed change in triagetoml for PRs ✨~~ feat: valdiate proposed change(s) in triage.toml for PRs ✨ Oct 14, 2023

meysam81 changed the title ~~feat: valdiate proposed change(s) in triage.toml for PRs ✨~~ feat: valdiate proposed change(s) in triagebot.toml for PRs ✨ Oct 14, 2023

ehuss reviewed Oct 17, 2023

View reviewed changes

meysam81 marked this pull request as draft October 23, 2023 06:46

meysam81 force-pushed the master branch from 6d3e115 to a624953 Compare November 12, 2023 13:03

meysam81 marked this pull request as ready for review November 12, 2023 13:13

meysam81 requested review from Mark-Simulacrum, apiraino and ehuss November 12, 2023 13:13

ehuss reviewed Nov 13, 2023

View reviewed changes

apiraino reviewed Nov 14, 2023

View reviewed changes

meysam81 requested review from apiraino and ehuss November 15, 2023 02:08

ehuss changed the title ~~feat: valdiate proposed change(s) in triagebot.toml for PRs ✨~~ feat: validate proposed change(s) in triagebot.toml for PRs ✨ Nov 16, 2023

ehuss added 2 commits January 21, 2024 16:26

Update toml to 0.8

11aecef

Add serde deny_unknown_fields to Config

522cfb1

This adds deny_unknown_fields to Config so that any typo mistakes will be caught by the config validation.

ehuss force-pushed the master branch from 6724989 to 466f30e Compare January 22, 2024 00:40

feat: valdiate proposed change in triagetoml for PRs ✨

252acb2

fixes rust-lang/rust#106104

ehuss force-pushed the master branch from 466f30e to 252acb2 Compare January 22, 2024 00:42

ehuss approved these changes Jan 22, 2024

View reviewed changes

ehuss merged commit 7a497a0 into rust-lang:master Jan 22, 2024

Conversation

meysam81 commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meysam81 commented Oct 8, 2023

Uh oh!

rustbot commented Oct 8, 2023

Uh oh!

meysam81 commented Oct 8, 2023

Uh oh!

apiraino Oct 8, 2023

Choose a reason for hiding this comment

Uh oh!

apiraino Oct 8, 2023

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum Oct 8, 2023

Choose a reason for hiding this comment

Uh oh!

meysam81 commented Oct 14, 2023

Uh oh!

ehuss Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

ehuss commented Oct 17, 2023

Uh oh!

ehuss commented Nov 13, 2023

Uh oh!

ehuss Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

meysam81 commented Nov 14, 2023

Uh oh!

apiraino Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

ehuss commented Nov 16, 2023

Uh oh!

ehuss commented Nov 16, 2023

Uh oh!

ehuss commented Nov 17, 2023

Uh oh!

ehuss commented Jan 22, 2024

Uh oh!

apiraino commented Jan 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

meysam81 commented Oct 8, 2023 •

edited

Loading