From d4849ccda1d3980b3ffce99179a20afb2d868965 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Tue, 21 Apr 2026 12:44:32 +0200 Subject: [PATCH] compliance: Add support for checking singular commit (#54369) This helps with two things: Testing changes locally against real commits as well as laying the groundwork for making things less convoluted for the GitHub worker. Also removes some useless wrapping left from an earlier direction. Release Notes: - N/A --- .github/workflows/compliance_check.yml | 2 +- .github/workflows/release.yml | 4 +- tooling/compliance/src/checks.rs | 29 +++++--- tooling/compliance/src/git.rs | 36 ++++++++++ tooling/compliance/src/github.rs | 19 +---- tooling/xtask/src/tasks/compliance.rs | 73 +++++++++++++++----- tooling/xtask/src/tasks/workflows/release.rs | 2 +- 7 files changed, 116 insertions(+), 49 deletions(-) diff --git a/.github/workflows/compliance_check.yml b/.github/workflows/compliance_check.yml index 144185f95b..f662dfd707 100644 --- a/.github/workflows/compliance_check.yml +++ b/.github/workflows/compliance_check.yml @@ -36,7 +36,7 @@ jobs: - id: run-compliance-check name: release::add_compliance_steps::run_compliance_check run: | - cargo xtask compliance "$LATEST_TAG" --branch main --report-path "compliance-report-${GITHUB_REF_NAME}.md" + cargo xtask compliance version "$LATEST_TAG" --branch main --report-path "compliance-report-${GITHUB_REF_NAME}.md" env: GITHUB_APP_ID: ${{ secrets.ZED_ZIPPY_APP_ID }} GITHUB_APP_KEY: ${{ secrets.ZED_ZIPPY_APP_PRIVATE_KEY }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8081955920..17e121b995 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -309,7 +309,7 @@ jobs: - id: run-compliance-check name: release::add_compliance_steps::run_compliance_check run: | - cargo xtask compliance "$GITHUB_REF_NAME" --report-path "compliance-report-${GITHUB_REF_NAME}.md" + cargo xtask compliance version "$GITHUB_REF_NAME" --report-path "compliance-report-${GITHUB_REF_NAME}.md" env: GITHUB_APP_ID: ${{ secrets.ZED_ZIPPY_APP_ID }} GITHUB_APP_KEY: ${{ secrets.ZED_ZIPPY_APP_PRIVATE_KEY }} @@ -678,7 +678,7 @@ jobs: - id: run-compliance-check name: release::add_compliance_steps::run_compliance_check run: | - cargo xtask compliance "$GITHUB_REF_NAME" --report-path "compliance-report-${GITHUB_REF_NAME}.md" + cargo xtask compliance version "$GITHUB_REF_NAME" --report-path "compliance-report-${GITHUB_REF_NAME}.md" env: GITHUB_APP_ID: ${{ secrets.ZED_ZIPPY_APP_ID }} GITHUB_APP_KEY: ${{ secrets.ZED_ZIPPY_APP_PRIVATE_KEY }} diff --git a/tooling/compliance/src/checks.rs b/tooling/compliance/src/checks.rs index cda37a84d2..409df5394f 100644 --- a/tooling/compliance/src/checks.rs +++ b/tooling/compliance/src/checks.rs @@ -1,11 +1,11 @@ -use std::{fmt, ops::Not as _}; +use std::{fmt, ops::Not as _, rc::Rc}; use itertools::Itertools as _; use crate::{ git::{CommitDetails, CommitList}, github::{ - CommitAuthor, GithubClient, GithubLogin, PullRequestComment, PullRequestData, + CommitAuthor, GithubApiClient, GithubLogin, PullRequestComment, PullRequestData, PullRequestReview, Repository, ReviewState, }, report::Report, @@ -87,19 +87,28 @@ impl> From for ReviewFailure { } } -pub struct Reporter<'a> { +pub struct Reporter { commits: CommitList, - github_client: &'a GithubClient, + github_client: Rc, } -impl<'a> Reporter<'a> { - pub fn new(commits: CommitList, github_client: &'a GithubClient) -> Self { +impl Reporter { + pub fn new(commits: CommitList, github_client: Rc) -> Self { Self { commits, github_client, } } + pub async fn result_for_commit( + commit: CommitDetails, + github_client: Rc, + ) -> ReviewResult { + Self::new(Default::default(), github_client) + .check_commit(&commit) + .await + } + /// Method that checks every commit for compliance pub async fn check_commit( &self, @@ -293,8 +302,8 @@ mod tests { use crate::git::{CommitDetails, CommitList, CommitSha}; use crate::github::{ - AuthorsForCommits, GithubApiClient, GithubClient, GithubLogin, GithubUser, - PullRequestComment, PullRequestData, PullRequestReview, Repository, ReviewState, + AuthorsForCommits, GithubApiClient, GithubLogin, GithubUser, PullRequestComment, + PullRequestData, PullRequestReview, Repository, ReviewState, }; use super::{Reporter, ReviewFailure, ReviewSuccess}; @@ -466,8 +475,8 @@ mod tests { commit_authors_json: self.commit_authors_json, org_members: self.org_members, }; - let client = GithubClient::new(Rc::new(mock)); - let reporter = Reporter::new(CommitList::default(), &client); + let client = Rc::new(mock); + let reporter = Reporter::new(CommitList::default(), client); reporter.check_commit(&self.commit).await } } diff --git a/tooling/compliance/src/git.rs b/tooling/compliance/src/git.rs index e2581b1c7f..f5b1a9c329 100644 --- a/tooling/compliance/src/git.rs +++ b/tooling/compliance/src/git.rs @@ -138,6 +138,18 @@ impl CommitDetails { } } +impl FromStr for CommitDetails { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + CommitList::from_str(s).and_then(|list| { + list.into_iter() + .next() + .ok_or_else(|| anyhow!("No commit found")) + }) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct Committer { name: String, @@ -318,6 +330,30 @@ impl FromStr for VersionTagList { } } +pub struct InfoForCommit { + sha: String, +} + +impl InfoForCommit { + pub fn new(sha: impl ToString) -> Self { + Self { + sha: sha.to_string(), + } + } +} + +impl Subcommand for InfoForCommit { + type ParsedOutput = CommitDetails; + + fn args(&self) -> impl IntoIterator { + [ + "log".to_string(), + format!("--pretty=format:{}", CommitDetails::FORMAT_STRING), + format!("{sha}~1..{sha}", sha = self.sha), + ] + } +} + pub struct CommitsFromVersionToVersion { version_tag: VersionTag, branch: String, diff --git a/tooling/compliance/src/github.rs b/tooling/compliance/src/github.rs index 29e22c92fc..edd2e4ea2e 100644 --- a/tooling/compliance/src/github.rs +++ b/tooling/compliance/src/github.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, collections::HashMap, fmt, ops::Not, rc::Rc}; +use std::{borrow::Cow, collections::HashMap, fmt, ops::Not}; use anyhow::Result; use derive_more::Deref; @@ -210,23 +210,6 @@ pub trait GithubApiClient { ) -> Result<()>; } -#[derive(Deref)] -pub struct GithubClient { - api: Rc, -} - -impl GithubClient { - pub fn new(api: Rc) -> Self { - Self { api } - } - - #[cfg(feature = "octo-client")] - pub async fn for_app_in_repo(app_id: u64, app_private_key: &str, org: &str) -> Result { - let client = OctocrabClient::new(app_id, app_private_key, org).await?; - Ok(Self::new(Rc::new(client))) - } -} - pub mod graph_ql { use anyhow::{Context as _, Result}; use itertools::Itertools as _; diff --git a/tooling/xtask/src/tasks/compliance.rs b/tooling/xtask/src/tasks/compliance.rs index fbe06383cf..46c40fb82c 100644 --- a/tooling/xtask/src/tasks/compliance.rs +++ b/tooling/xtask/src/tasks/compliance.rs @@ -1,20 +1,37 @@ -use std::path::PathBuf; +use std::{path::PathBuf, rc::Rc}; use anyhow::{Context, Result}; -use clap::Parser; +use clap::{Parser, Subcommand}; use compliance::{ checks::Reporter, - git::{CommitsFromVersionToVersion, GetVersionTags, GitCommand, VersionTag}, - github::{GithubClient, Repository}, + git::{CommitsFromVersionToVersion, GetVersionTags, GitCommand, InfoForCommit, VersionTag}, + github::{GithubApiClient as _, OctocrabClient, Repository}, report::ReportReviewSummary, }; #[derive(Parser)] -pub struct ComplianceArgs { +pub(crate) struct ComplianceArgs { + #[clap(subcommand)] + mode: ComplianceMode, +} + +#[derive(Subcommand)] +pub(crate) enum ComplianceMode { + // Check compliance for all commits between two version tags + Version(VersionArgs), + // Check compliance for a single commit + Single { + // The full commit SHA to check + commit_sha: String, + }, +} + +#[derive(Parser)] +pub(crate) struct VersionArgs { #[arg(value_parser = VersionTag::parse)] // The version to be on the lookout for - pub(crate) version_tag: VersionTag, + version_tag: VersionTag, #[arg(long)] // The markdown file to write the compliance report to report_path: PathBuf, @@ -23,7 +40,7 @@ pub struct ComplianceArgs { branch: Option, } -impl ComplianceArgs { +impl VersionArgs { pub(crate) fn version_tag(&self) -> &VersionTag { &self.version_tag } @@ -39,6 +56,35 @@ async fn check_compliance_impl(args: ComplianceArgs) -> Result<()> { let app_id = std::env::var("GITHUB_APP_ID").context("Missing GITHUB_APP_ID")?; let key = std::env::var("GITHUB_APP_KEY").context("Missing GITHUB_APP_KEY")?; + let client = Rc::new( + OctocrabClient::new( + app_id.parse().context("Failed to parse app ID as int")?, + key.as_ref(), + Repository::ZED.owner(), + ) + .await?, + ); + + println!("Initialized GitHub client for app ID {app_id}"); + + let args = match args.mode { + ComplianceMode::Version(version) => version, + ComplianceMode::Single { commit_sha } => { + let commit = GitCommand::run(InfoForCommit::new(&commit_sha))?; + + return match Reporter::result_for_commit(commit, client).await { + Ok(review_success) => { + println!("Check for commit {commit_sha} succeeded. Result: {review_success}",); + Ok(()) + } + + Err(review_failure) => Err(anyhow::anyhow!( + "Check for commit {commit_sha} failed. Result: {review_failure}" + )), + }; + } + }; + let tag = args.version_tag(); let previous_version = GitCommand::run(GetVersionTags)? @@ -69,16 +115,9 @@ async fn check_compliance_impl(args: ComplianceArgs) -> Result<()> { println!("Checking commit range {range}, {} total", commits.len()); - let client = GithubClient::for_app_in_repo( - app_id.parse().context("Failed to parse app ID as int")?, - key.as_ref(), - Repository::ZED.owner(), - ) - .await?; - - println!("Initialized GitHub client for app ID {app_id}"); - - let report = Reporter::new(commits, &client).generate_report().await?; + let report = Reporter::new(commits, client.clone()) + .generate_report() + .await?; println!( "Generated report for version {}", diff --git a/tooling/xtask/src/tasks/workflows/release.rs b/tooling/xtask/src/tasks/workflows/release.rs index e3e0fb78a8..76895a37e3 100644 --- a/tooling/xtask/src/tasks/workflows/release.rs +++ b/tooling/xtask/src/tasks/workflows/release.rs @@ -185,7 +185,7 @@ pub(crate) fn add_compliance_steps( fn run_compliance_check(context: &ComplianceContext) -> (Step, StepOutput) { let job = named::bash( formatdoc! {r#" - cargo xtask compliance {target} --report-path "{COMPLIANCE_REPORT_PATH}" + cargo xtask compliance version {target} --report-path "{COMPLIANCE_REPORT_PATH}" "#, target = if context.tag_source().is_some() { r#""$LATEST_TAG" --branch main"# } else { r#""$GITHUB_REF_NAME""# }, }