Repository navigation
Validate triagebot.toml in tidy/CI #106104
Description
Activity
Mentoring instructions: Add a new check to tidy around here that parses
triagebot.tomlto make sure it's valid toml:rust/src/tools/tidy/src/lib.rs
Lines 41 to 58 in 17395b4
See the existing tidy files for an example of how to write a tidy check.Hi, I'm a new contributor here and thought of picking this as my first issue. From what I understand, a new crate (something like
triagebot.rs) needs to be made for this issue right? Where we can parse the toml and see if it is valid.@aadityadhruv a new module, not a new crate. The module should be part of
tidyso check runs onx test tidy.Right, that makes sense. Thank you!
Hi, sorry for the late response, I was out traveling. I'm currently confused about a certain thing. Currently what I am trying to do is manually check the file for valid syntax, by going over it line by line. Is this the correct method to do this, or should I be using an already existing crate like
toml? I would assume that extra crate dependencies aren't probably wanted.Also, I was wondering something about Path in the check function. For this module, would I need to check/filter any paths like some other modules do? I was assuming that I will simply open path +
'tiragebot.toml'and read that file.Any help is appreciated!
should I be using an already existing crate like toml?
Yes, please use toml, don't use regex or anything like that.
I personally would prefer to see this implemented in triagebot itself instead of tidy. That would make it work for all repositories. It should be fairly trivial to add a handler that checks for a PR that modifies triagebot.toml and to validate it, and post a comment if it is not correct. It can even use the
Configtype to verify that the actual structure matches. Bonus points would to also use serde-ignored to enforce that unknown fields aren't included.Reacted by Mark Rousskov, jyn and Meysam@aadityadhruv are you still working on this?
Hi, yeah I'm sorry, I'm still working on it. I've been pretty busy the last couple of days, I'll get it done this week itself. Hope that's ok!
Hi, yeah I'm sorry, I'm still working on it. I've been pretty busy the last couple of days, I'll get it done this week itself. Hope that's ok!
Yeah! it's alright
I personally would prefer to see this implemented in triagebot itself instead of tidy. That would make it work for all repositories. It should be fairly trivial to add a handler that checks for a PR that modifies triagebot.toml and to validate it, and post a comment if it is not correct. It can even use the
Configtype to verify that the actual structure matches. Bonus points would to also use serde-ignored to enforce that unknown fields aren't included.Sorry for being slow with the issue, but along with being busy, I've been kinda stuck, and was unsure if I should ask for help. I am currently unsure if I should be doing work in
tidyor thetriagebotrepository. Should I first try to get thetidycheck working and then maybe open an issue on thetriagebotrepo?Here is the code I had written (it does not work since I do not know what the configuration of the toml is):
use crate::walk::{filter_dirs, walk}; use serde::Deserialize; use std::path::Path; #[derive(Deserialize)] struct Config {} pub fn check(path: &Path, bad: &mut bool) { walk(path, &mut |path| filter_dirs(path), &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap(); if filename != "triagebot.toml" { return; } let conf: Result<Config, toml::de::Error> = toml::from_str(contents); match conf { Ok(_) => {} Err(_err) => { tidy_error!(bad, "{} is an invalid toml file", file.display()) } } }); }
I am unsure what would be the right next step here.
2 remaining items
Here is a PR for the same: #106559
I personally would prefer to see this implemented in triagebot itself instead of tidy. That would make it work for all repositories. It should be fairly trivial to add a handler that checks for a PR that modifies triagebot.toml and to validate it, and post a comment if it is not correct. It can even use the
Configtype to verify that the actual structure matches. Bonus points would to also use serde-ignored to enforce that unknown fields aren't included.Ah, that wouldn't have been caught actually - the toml syntax was valid, the file just didn't exist in the repository. Checking if the glob has at least one match seems possible.
I see the PR up but not sure if anyone is still working on the requested changes?
Yeah I'm currently working on it in the triagebot repo, I haven't opened a PR/Issue yet for it.
- added a commit that references this issue
on Oct 8, 2023 Can someone please review this change: rust-lang/triagebot#1730
- added a commit that references this issue
on Nov 12, 2023 - added a commit that references this issue
on Jan 22, 2024
Would've prevented #106102, which broke in #105661.
We should at least validate the toml file is valid syntax, but ideally we'd validate more context-specific stuff.