From 36e5c272c96c3922300edeeafc9a6ffb0c39990c Mon Sep 17 00:00:00 2001 From: Urgau Date: Sat, 23 Mar 2024 23:43:43 +0100 Subject: [PATCH 1/8] Fix rfcbot and also deserialize concerns --- src/rfcbot.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/rfcbot.rs b/src/rfcbot.rs index 93770a35..0ab390d5 100644 --- a/src/rfcbot.rs +++ b/src/rfcbot.rs @@ -24,10 +24,16 @@ pub struct Review { pub approved: bool, } #[derive(Serialize, Deserialize, Debug, Clone)] +pub struct Concern { + pub name: String, + pub comment: StatusComment, + pub reviewer: Reviewer, +} +#[derive(Serialize, Deserialize, Debug, Clone)] pub struct FCPIssue { pub id: u32, pub number: u32, - pub fk_milestone: Option, + pub fk_milestone: Option, pub fk_user: u32, pub fk_assignee: Option, pub open: bool, @@ -57,6 +63,7 @@ pub struct StatusComment { pub struct FullFCP { pub fcp: FCP, pub reviews: Vec, + pub concerns: Vec, pub issue: FCPIssue, pub status_comment: StatusComment, } From 03d9c8444a5b23a5356ec1649bb69d4d63ae05e7 Mon Sep 17 00:00:00 2001 From: Urgau Date: Sat, 23 Mar 2024 23:48:10 +0100 Subject: [PATCH 2/8] Add more details to relevant FCPs steps --- src/actions.rs | 14 +++++++++++++- src/github.rs | 27 ++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/actions.rs b/src/actions.rs index 73557078..2f26dea4 100644 --- a/src/actions.rs +++ b/src/actions.rs @@ -9,6 +9,7 @@ use tera::{Context, Tera}; use crate::{ github::{self, GithubClient, Repository}, http_client::{CompilerMeeting, HttpClient}, + rfcbot, }; #[async_trait] @@ -54,12 +55,22 @@ pub struct IssueDecorator { pub mcp_details: Option, } +#[derive(Serialize, Deserialize, Debug)] +pub struct FCPConcernDetails { + pub name: String, + pub reviewer_login: String, + pub concern_url: String, +} + #[derive(Serialize, Deserialize, Debug)] pub struct FCPDetails { pub bot_tracking_comment_html_url: String, pub bot_tracking_comment_content: String, pub initiating_comment_html_url: String, pub initiating_comment_content: String, + pub disposition: String, + pub pending_reviewers: Vec, + pub concerns: Vec, } #[derive(Serialize, Deserialize, Debug)] @@ -130,6 +141,7 @@ impl<'a> Action for Step<'a> { let query = query.clone(); handles.push(tokio::task::spawn(async move { let _permit = semaphore.acquire().await?; + let fcps_groups = ["proposed_fcp", "in_pre_fcp", "in_fcp"]; let mcps_groups = [ "mcp_new_not_seconded", "mcp_old_not_seconded", @@ -140,7 +152,7 @@ impl<'a> Action for Step<'a> { let issues = query .query( &repository, - name == "proposed_fcp", + fcps_groups.contains(&name.as_str()), mcps_groups.contains(&name.as_str()) && repository.full_name.contains("rust-lang/compiler-team"), &gh, diff --git a/src/github.rs b/src/github.rs index 0a81a263..06475651 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1712,7 +1712,9 @@ impl<'q> IssuesQuery for Query<'q> { .with_context(|| "Unable to get issues.")?; let fcp_map = if include_fcp_details { - crate::rfcbot::get_all_fcps().await? + crate::rfcbot::get_all_fcps() + .await + .with_context(|| "Unable to get all fcps from rfcbot.")? } else { HashMap::new() }; @@ -1750,6 +1752,29 @@ impl<'q> IssuesQuery for Query<'q> { bot_tracking_comment_content, initiating_comment_html_url: init_comment.html_url.clone(), initiating_comment_content: quote_reply(&init_comment.body), + disposition: fcp + .fcp + .disposition + .as_deref() + .unwrap_or("") + .to_string(), + pending_reviewers: fcp + .reviews + .iter() + .filter_map(|r| (!r.approved).then(|| r.reviewer.clone())) + .collect(), + concerns: fcp + .concerns + .iter() + .map(|c| crate::actions::FCPConcernDetails { + name: c.name.clone(), + reviewer_login: c.reviewer.login.clone(), + concern_url: format!( + "{}#issuecomment-{}", + issue.html_url, c.comment.id + ), + }) + .collect(), }) } else { None From 402d4f8d9ed298653adb8fa129d3b8d5de08232d Mon Sep 17 00:00:00 2001 From: Urgau Date: Sat, 23 Mar 2024 23:48:52 +0100 Subject: [PATCH 3/8] Use rfcbot-ishy style for fcps in the prioritization agenda --- templates/_issues_rfcbot.tt | 15 +++++++++++++++ templates/prioritization_agenda.tt | 5 +++-- 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 templates/_issues_rfcbot.tt diff --git a/templates/_issues_rfcbot.tt b/templates/_issues_rfcbot.tt new file mode 100644 index 00000000..4ed3733c --- /dev/null +++ b/templates/_issues_rfcbot.tt @@ -0,0 +1,15 @@ +{% import "_issue.tt" as issue %} + +{% macro render(issues, indent="", empty="None.") %} +{%- for issue in issues %} +{%- if issue.fcp_details is object %} +{{indent}}- {{issue.fcp_details.disposition}}: [{{issue.title}} ({{issue.repo_name}}#{{issue.number}})]({{issue.fcp_details.bot_tracking_comment_html_url}}) +{{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @**{{reviewer.login}}**{%else%} no pending checkboxs{% endfor %} +{{indent}}{{indent}}-{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%} +{% else %} +{{indent}}- "{{issue.title}}" {{issue.repo_name}}#{{issue.number}} +{%endif-%} +{%else%} +{{empty}} +{%endfor-%} +{% endmacro %} diff --git a/templates/prioritization_agenda.tt b/templates/prioritization_agenda.tt index 47763e34..91d11cd7 100644 --- a/templates/prioritization_agenda.tt +++ b/templates/prioritization_agenda.tt @@ -1,5 +1,6 @@ {% import "_issues.tt" as issues %} {% import "_meetings.tt" as meetings %} +{% import "_issues_rfcbot.tt" as issues_rfcbot %} --- tags: weekly, rustc @@ -28,9 +29,9 @@ note_id: xxx - Old MCPs (not seconded, take a look) {{-issues::render(issues=mcp_old_not_seconded, indent=" ", with_age=true, empty="No old proposals this time.")}} - Pending FCP requests (check your boxes!) -{{-issues::render(issues=in_pre_fcp, indent=" ", empty="No pending FCP requests this time.")}} +{{-issues_rfcbot::render(issues=in_pre_fcp, indent=" ", empty="No pending FCP requests this time.")}} - Things in FCP (make sure you're good with it) -{{-issues::render(issues=in_fcp, indent=" ", empty="No FCP requests this time.")}} +{{-issues_rfcbot::render(issues=in_fcp, indent=" ", empty="No FCP requests this time.")}} - Accepted MCPs {{-issues::render(issues=mcp_accepted, indent=" ", empty="No new accepted proposals this time.")}} - MCPs blocked on unresolved concerns From 97baa0b1de1f10e9d438fe931ab8c970309f47c2 Mon Sep 17 00:00:00 2001 From: Urgau Date: Sun, 24 Mar 2024 15:40:57 +0100 Subject: [PATCH 4/8] Use Zulip IDs instead of GitHub login in rfcbot issues --- src/actions.rs | 9 +++++++-- src/github.rs | 21 ++++++++++++++++++++- src/rfcbot.rs | 2 +- templates/_issues_rfcbot.tt | 2 +- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/actions.rs b/src/actions.rs index 2f26dea4..c010deb7 100644 --- a/src/actions.rs +++ b/src/actions.rs @@ -9,7 +9,6 @@ use tera::{Context, Tera}; use crate::{ github::{self, GithubClient, Repository}, http_client::{CompilerMeeting, HttpClient}, - rfcbot, }; #[async_trait] @@ -62,6 +61,12 @@ pub struct FCPConcernDetails { pub concern_url: String, } +#[derive(Serialize, Deserialize, Debug)] +pub struct FCPReviewerDetails { + pub github_login: String, + pub zulip_id: Option, +} + #[derive(Serialize, Deserialize, Debug)] pub struct FCPDetails { pub bot_tracking_comment_html_url: String, @@ -69,7 +74,7 @@ pub struct FCPDetails { pub initiating_comment_html_url: String, pub initiating_comment_content: String, pub disposition: String, - pub pending_reviewers: Vec, + pub pending_reviewers: Vec, pub concerns: Vec, } diff --git a/src/github.rs b/src/github.rs index 06475651..ce960d6b 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1719,6 +1719,12 @@ impl<'q> IssuesQuery for Query<'q> { HashMap::new() }; + let zulip_map = if include_fcp_details { + Some(crate::team_data::zulip_map(client).await?) + } else { + None + }; + let mut issues_decorator = Vec::new(); let re = regex::Regex::new("https://github.com/rust-lang/|/").unwrap(); let re_zulip_link = regex::Regex::new(r"\[stream\]:\s").unwrap(); @@ -1761,7 +1767,20 @@ impl<'q> IssuesQuery for Query<'q> { pending_reviewers: fcp .reviews .iter() - .filter_map(|r| (!r.approved).then(|| r.reviewer.clone())) + .filter_map(|r| { + (!r.approved).then(|| crate::actions::FCPReviewerDetails { + github_login: r.reviewer.login.clone(), + zulip_id: zulip_map + .as_ref() + .map(|map| { + map.users + .iter() + .find(|&(_, &github)| github == r.reviewer.id) + .map(|v| *v.0) + }) + .flatten(), + }) + }) .collect(), concerns: fcp .concerns diff --git a/src/rfcbot.rs b/src/rfcbot.rs index 0ab390d5..e3874548 100644 --- a/src/rfcbot.rs +++ b/src/rfcbot.rs @@ -15,7 +15,7 @@ pub struct FCP { } #[derive(Serialize, Deserialize, Debug, Clone)] pub struct Reviewer { - pub id: u32, + pub id: u64, pub login: String, } #[derive(Serialize, Deserialize, Debug, Clone)] diff --git a/templates/_issues_rfcbot.tt b/templates/_issues_rfcbot.tt index 4ed3733c..dce2d594 100644 --- a/templates/_issues_rfcbot.tt +++ b/templates/_issues_rfcbot.tt @@ -4,7 +4,7 @@ {%- for issue in issues %} {%- if issue.fcp_details is object %} {{indent}}- {{issue.fcp_details.disposition}}: [{{issue.title}} ({{issue.repo_name}}#{{issue.number}})]({{issue.fcp_details.bot_tracking_comment_html_url}}) -{{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @**{{reviewer.login}}**{%else%} no pending checkboxs{% endfor %} +{{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @**|{{reviewer.zulip_id}}**{%else%} no pending checkboxs{% endfor %} {{indent}}{{indent}}-{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%} {% else %} {{indent}}- "{{issue.title}}" {{issue.repo_name}}#{{issue.number}} From 189ae3d9814ce55be2d0fac477ecbd773529969f Mon Sep 17 00:00:00 2001 From: Urgau Date: Thu, 28 Mar 2024 22:25:48 +0100 Subject: [PATCH 5/8] Only mention people under very specific circumstances --- src/actions.rs | 1 + src/github.rs | 15 +++++++++++++++ templates/_issues_rfcbot.tt | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/actions.rs b/src/actions.rs index c010deb7..5cdfe86f 100644 --- a/src/actions.rs +++ b/src/actions.rs @@ -74,6 +74,7 @@ pub struct FCPDetails { pub initiating_comment_html_url: String, pub initiating_comment_content: String, pub disposition: String, + pub should_mention: bool, pub pending_reviewers: Vec, pub concerns: Vec, } diff --git a/src/github.rs b/src/github.rs index ce960d6b..274500bf 100644 --- a/src/github.rs +++ b/src/github.rs @@ -361,6 +361,8 @@ pub struct Comment { pub html_url: String, pub user: User, #[serde(alias = "submitted_at")] // for pull request reviews + pub created_at: chrono::DateTime, + #[serde(alias = "submitted_at")] // for pull request reviews pub updated_at: chrono::DateTime, #[serde(default, rename = "state")] pub pr_review_state: Option, @@ -1753,6 +1755,18 @@ impl<'q> IssuesQuery for Query<'q> { .get_comment(&client, fk_initiating_comment.try_into()?) .await?; + // To avoid constant (and counter-productive) pings we will only ping when + // - we are 2 weeks into the FCP + // - or when there are no concerns and we are at least 4 weeks into the FCP. + // + // FIXME: This should get T-compiler approval before being enabled by default + let should_mention = std::env::var("TRIAGEBOT_COMPILER_MENTION").is_ok() && { + let now = chrono::offset::Utc::now(); + let time_diff = now - init_comment.created_at; + time_diff.num_weeks() == 2 + || (time_diff.num_weeks() >= 4 && fcp.concerns.is_empty()) + }; + Some(crate::actions::FCPDetails { bot_tracking_comment_html_url, bot_tracking_comment_content, @@ -1764,6 +1778,7 @@ impl<'q> IssuesQuery for Query<'q> { .as_deref() .unwrap_or("") .to_string(), + should_mention, pending_reviewers: fcp .reviews .iter() diff --git a/templates/_issues_rfcbot.tt b/templates/_issues_rfcbot.tt index dce2d594..cdb117e4 100644 --- a/templates/_issues_rfcbot.tt +++ b/templates/_issues_rfcbot.tt @@ -4,7 +4,7 @@ {%- for issue in issues %} {%- if issue.fcp_details is object %} {{indent}}- {{issue.fcp_details.disposition}}: [{{issue.title}} ({{issue.repo_name}}#{{issue.number}})]({{issue.fcp_details.bot_tracking_comment_html_url}}) -{{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @**|{{reviewer.zulip_id}}**{%else%} no pending checkboxs{% endfor %} +{{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @{% if issue.fcp_details.should_mention %}{% else %}_{% endif %}**|{{reviewer.zulip_id}}**{%else%} no pending checkboxs{% endfor %} {{indent}}{{indent}}-{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%} {% else %} {{indent}}- "{{issue.title}}" {{issue.repo_name}}#{{issue.number}} From 0205e145761f256b0e39c24b6d919d7d1bcbac00 Mon Sep 17 00:00:00 2001 From: Urgau Date: Thu, 4 Apr 2024 18:39:48 +0200 Subject: [PATCH 6/8] Fix fallback when we don't any FCP details --- templates/_issues_rfcbot.tt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/_issues_rfcbot.tt b/templates/_issues_rfcbot.tt index cdb117e4..c74cbbc7 100644 --- a/templates/_issues_rfcbot.tt +++ b/templates/_issues_rfcbot.tt @@ -7,8 +7,8 @@ {{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @{% if issue.fcp_details.should_mention %}{% else %}_{% endif %}**|{{reviewer.zulip_id}}**{%else%} no pending checkboxs{% endfor %} {{indent}}{{indent}}-{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%} {% else %} -{{indent}}- "{{issue.title}}" {{issue.repo_name}}#{{issue.number}} -{%endif-%} +{{indent}}- {{issue::render(issue=issue)}} +{%- endif -%} {%else%} {{empty}} {%endfor-%} From d5fbc0f1d4db76d27c37318a8c7d27c3e6b57586 Mon Sep 17 00:00:00 2001 From: Urgau <3616612+Urgau@users.noreply.github.com> Date: Mon, 15 Apr 2024 18:28:40 +0200 Subject: [PATCH 7/8] Remove (disable-by-default) mentions until formal decision Co-authored-by: apiraino --- src/github.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/github.rs b/src/github.rs index 274500bf..dabe3af5 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1755,18 +1755,9 @@ impl<'q> IssuesQuery for Query<'q> { .get_comment(&client, fk_initiating_comment.try_into()?) .await?; - // To avoid constant (and counter-productive) pings we will only ping when - // - we are 2 weeks into the FCP - // - or when there are no concerns and we are at least 4 weeks into the FCP. - // - // FIXME: This should get T-compiler approval before being enabled by default - let should_mention = std::env::var("TRIAGEBOT_COMPILER_MENTION").is_ok() && { - let now = chrono::offset::Utc::now(); - let time_diff = now - init_comment.created_at; - time_diff.num_weeks() == 2 - || (time_diff.num_weeks() >= 4 && fcp.concerns.is_empty()) - }; - + // TODO: agree with the team(s) a policy to emit actual mentions to remind FCP + // voting member to cast their vote + let should_mention = false; Some(crate::actions::FCPDetails { bot_tracking_comment_html_url, bot_tracking_comment_content, From 7506b21728768257d5400c1bfdc05ceead81e408 Mon Sep 17 00:00:00 2001 From: Urgau Date: Tue, 23 Apr 2024 19:00:41 +0200 Subject: [PATCH 8/8] Prefix rfcbot concerns with "concerns:" text --- templates/_issues_rfcbot.tt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/_issues_rfcbot.tt b/templates/_issues_rfcbot.tt index c74cbbc7..7c4d8dc2 100644 --- a/templates/_issues_rfcbot.tt +++ b/templates/_issues_rfcbot.tt @@ -5,7 +5,7 @@ {%- if issue.fcp_details is object %} {{indent}}- {{issue.fcp_details.disposition}}: [{{issue.title}} ({{issue.repo_name}}#{{issue.number}})]({{issue.fcp_details.bot_tracking_comment_html_url}}) {{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @{% if issue.fcp_details.should_mention %}{% else %}_{% endif %}**|{{reviewer.zulip_id}}**{%else%} no pending checkboxs{% endfor %} -{{indent}}{{indent}}-{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%} +{{indent}}{{indent}}-{% if issue.fcp_details.concerns|length > 0 %} concerns:{% endif %}{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%} {% else %} {{indent}}- {{issue::render(issue=issue)}} {%- endif -%}