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.

Sign up for GitHub

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

Guard against overflowing GitHub ID comments #1821

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Guard against overflowing GitHub ID comments #1821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ impl Issue {
self.state == IssueState::Open
}

pub async fn get_comment(&self, client: &GithubClient, id: u64) -> anyhow::Result<Comment> {
pub async fn get_comment(&self, client: &GithubClient, id: i32) -> anyhow::Result<Comment> {
let comment_url = format!("{}/issues/comments/{}", self.repository().url(client), id);
let comment = client.json(client.get(&comment_url)).await?;
Ok(comment)
Expand Down Expand Up @@ -1825,20 +1825,34 @@ impl<'q> IssuesQuery for Query<'q> {
issue.html_url, fcp.fcp.fk_bot_tracking_comment
);
let bot_tracking_comment_content = quote_reply(&fcp.status_comment.body);

let fk_initiating_comment = fcp.fcp.fk_initiating_comment;
let init_comment = issue
.get_comment(&client, fk_initiating_comment.try_into()?)
.await?;
let (initiating_comment_html_url, initiating_comment_content) =
if u32::try_from(fk_initiating_comment).is_err() {
// We blew out the GH comment incremental counter (a i32 on their end)
// See: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/rfcbot.20asleep
log::debug!("Ignoring overflowed comment id from GitHub");
("".to_string(), "".to_string())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand why this is using empty strings. But I'll trust since I assume you do most of the agenda stuff that it works for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it's a bit of a hack and I am not either very fond of this. initiating_comment_html_url and initiating_comment_content are mandatory fields in FCPDetails, so the minimal change I could think of was to set them to an empty string rather than making them Option<String> and deal with a widerspread change in the codebase.

} else {
let comment = issue
.get_comment(&client, fk_initiating_comment.try_into()?)
.await
.with_context(|| {
format!(
"failed to get first comment id={} for fcp={}",
fk_initiating_comment, fcp.fcp.id
)
})?;
(comment.html_url, quote_reply(&comment.body))
};

// 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,
initiating_comment_html_url: init_comment.html_url.clone(),
initiating_comment_content: quote_reply(&init_comment.body),
initiating_comment_html_url,
initiating_comment_content,
disposition: fcp
.fcp
.disposition
Expand Down
6 changes: 3 additions & 3 deletions src/rfcbot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ pub struct FCP {
pub id: u32,
pub fk_issue: u32,
pub fk_initiator: u32,
pub fk_initiating_comment: u32,
pub fk_initiating_comment: i32,
pub disposition: Option<String>,
pub fk_bot_tracking_comment: u32,
pub fk_bot_tracking_comment: i32,
pub fcp_start: Option<String>,
pub fcp_closed: bool,
}
Expand Down Expand Up @@ -50,7 +50,7 @@ pub struct FCPIssue {

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct StatusComment {
pub id: u64,
pub id: i32,
pub fk_issue: u32,
pub fk_user: u32,
pub body: String,
Expand Down
Loading