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

Proposal for allowing @rust-log-analyzer silence to mute failures #73

Closed
tgross35 opened this issue Jul 2, 2023 · 2 comments · Fixed by #75
Closed

Proposal for allowing @rust-log-analyzer silence to mute failures #73

tgross35 opened this issue Jul 2, 2023 · 2 comments · Fixed by #75

Comments

@tgross35
Copy link
Contributor

tgross35 commented Jul 2, 2023

It would be nice to be able to make RLA a bit less annoying when you're actively watching your PR and know it is going to fail.

Proposed usage:

silence all announcements:
@rust-log-analyzer silence silence

silence a announcements about a single test:
@rust-log-analyzer silence  mingw-check

silence multiple:
@rust-log-analyzer silence  mingw-check, mingw-test-tidy

unsilence takes the same patterns as above:
@rust-log-analyzer silence

It seems like RLA is currently stateless. Doing this would require saving some information, but changes to this would be infrequent enough that a sqlite database could handle it easily;

CREATE TABLE silenced (
    pr_id INTEGER PRIMARY KEY,
    silence_all INTEGER NOT NULL DEFAULT FALSE,
    -- JSON array of specific silenced tests
    silenced TEXT
);

Behavior:

  • Accept the issue_comment webhook event.
    • If it contains silence but nothing else, upsert silence_all for that PR's ID
    • If it contains silence and some specific tests, update silence_all=false and those add those tests to a list in silenced
    • If it contains unsilence, do the opposite and delete the row if empty
  • Upon writing a comment, compare the failed test to the silencing rules and don't publish it if that test is silenced

I don't mind doing the work for this, but would appreciate some feedback before starting to make sure this sounds OK.

@Mark-Simulacrum
Copy link
Member

There's no trivial place to save the SQLite DB -- RLA runs on ephemeral containers. We do have some state (the trained index), which is persisted and loaded from S3: #71

It seems plausible that we could put a DB into S3 as well, but I wonder if perhaps a simpler design is to (for example) add a label to PRs "rla-silenced" or something like that? That would mean a binary silence rather than something more complex, but that seems like it owuld be sufficient for 99% of use cases anyway?

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 10, 2023

A label sounds very reasonable to me, and easier.

Is the only thing needed a check like this one? Query the PR labels then just return this function if that one is set

if !is_bors {
let pr_info = self.github.query_pr(&self.repo, pr)?;
if pr_info.head.sha != commit_sha {
info!("Build results outdated, skipping report.");
return Ok(());
}
}

tgross35 added a commit to tgross35/rust-log-analyzer that referenced this issue Aug 10, 2023
If a test is not run by Bors and the label `rla-silence` is applied to a
PR, do not post an update messsage. This will allow keeping RLA message
noise out of PRs that are expected to have a lot of churn.

Fixes rust-lang#73
tgross35 added a commit to tgross35/rust-log-analyzer that referenced this issue Aug 10, 2023
If a test is not run by Bors and the label `rla-silence` is applied to a
PR, do not post an update messsage. This will allow keeping RLA message
noise out of PRs that are expected to have a lot of churn.

Fixes rust-lang#73
@jdno jdno closed this as completed in #75 Nov 27, 2023
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 a pull request may close this issue.

2 participants