Skip vmm-tests for non-product PRs (Guide/**, repo_support/**/*.py)#2925
Skip vmm-tests for non-product PRs (Guide/**, repo_support/**/*.py)#2925
Conversation
Co-authored-by: mattkur <13772048+mattkur@users.noreply.github.com>
|
@copilot: this will need to work in local and ADO backends as well. Meaning: don't make this specific to the GH pipelines. |
…ment for draft check Co-authored-by: mattkur <13772048+mattkur@users.noreply.github.com>
| use flowey::node::prelude::*; | ||
|
|
||
| /// GitHub Actions expression suitable for use in a job-level `outputs:` block. |
There was a problem hiding this comment.
Why do we need a GitHub Actions expression?
There was a problem hiding this comment.
Removed entirely. The GitHub Actions expression is now generated internally by gh_set_job_output_from_env_var(name, env_var) from the env var name constant GH_ENV_IS_NON_PRODUCT. No raw expression is exported from the node. (commit 3897202)
flowey/flowey_core/src/pipeline.rs
Outdated
| /// (GitHub Actions only) Declare a job-level output. | ||
| /// | ||
| /// `name` is the output name; `expression` is a GitHub Actions expression | ||
| /// whose value becomes the output value, e.g. | ||
| /// `"${{ steps.my-step.outputs.my-key }}"` or `"${{ env.MY_VAR }}"`. | ||
| /// | ||
| /// The declared output is accessible to dependent jobs via | ||
| /// `needs.<this-job-id>.outputs.<name>`. | ||
| /// | ||
| /// See <https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idoutputs> | ||
| pub fn gh_set_job_output(self, name: impl AsRef<str>, expression: impl AsRef<str>) -> Self { | ||
| self.pipeline.jobs[self.job_idx] | ||
| .gh_job_outputs | ||
| .push((name.as_ref().into(), expression.as_ref().into())); | ||
| self | ||
| } |
There was a problem hiding this comment.
Can this be a {Read|Write}Var instead?
There was a problem hiding this comment.
Replaced gh_set_job_output(name, raw_expression) with gh_set_job_output_from_env_var(name, env_var) — the env var name (a typed string constant from the node) is now the input, and the ${{ env.VAR }} expression is generated automatically. The job output is declared as outputs: is_non_product: ${{ env.FLOWEY_IS_NON_PRODUCT }} in the regenerated YAML. (commit 3897202)
flowey/flowey_core/src/pipeline.rs
Outdated
| /// (GitHub Actions only) Overrides the auto-generated job ID used in the | ||
| /// GitHub Actions YAML. | ||
| /// | ||
| /// By default, Flowey assigns job IDs like `job0`, `job1`, etc. Use this | ||
| /// method to assign a stable, human-readable ID instead. This is useful | ||
| /// when other jobs need to reference this job's outputs via | ||
| /// `needs.<job-id>.outputs.<output-name>`. | ||
| /// | ||
| /// Job IDs must be unique across the pipeline. | ||
| pub fn gh_override_job_id(self, id: impl AsRef<str>) -> Self { | ||
| self.pipeline | ||
| .gh_job_id_overrides | ||
| .insert(self.job_idx, id.as_ref().into()); | ||
| self | ||
| } |
There was a problem hiding this comment.
Removed gh_override_job_id entirely. Added Pipeline::gh_job_id_of(handle) and Pipeline::ado_job_id_of(handle) methods that dynamically look up the auto-generated job IDs from handles. The classify job now gets an auto-generated ID (e.g. job18) and the vmm-tests conditions reference it via needs.job18.outputs.is_non_product. (commit 3897202)
There was a problem hiding this comment.
This whole file should be replaced by some flowey code that can then be consumed by all backends
There was a problem hiding this comment.
The action.yml is deleted. check_pr_changes::Node now uses emit_rust_step (GitHub/local) and emit_ado_step_with_inline_script (ADO) — pure Flowey code, no external GitHub Action, no GitHub API call. Classification is done via git diff against the base branch, which works across all three backends. (commit 3897202)
|
@copilot: In addition: please make sure that you update the Guide as well: (1) for the sections relevant to this work, and (2) elaborate on the flowey architecture + conventions, etc. |
…e gh_override_job_id Co-authored-by: mattkur <13772048+mattkur@users.noreply.github.com>
Added a new section to |
| To add a new non-product bucket, update **both** `is_non_product_path` (Rust, | ||
| used by the GitHub and local backends) and the equivalent `if` clause in the | ||
| ADO bash script inside `check_pr_changes::Node`. |
There was a problem hiding this comment.
Why is ADO so special? Smells like something we should promote architecturally.
There was a problem hiding this comment.
Fixed. The doc now says "edit non_product_config.toml" and the "How it works" table reflects that the same Rust classification code runs on all three backends — ADO is no longer special-cased. (commit 128ecdc)
| //! Classifies PR changed files to determine whether all changes fall within | ||
| //! approved non-product buckets (e.g. `Guide/**`, `repo_support/**/*.py`). | ||
| //! | ||
| //! This node works across all Flowey backends: | ||
| //! | ||
| //! - **GitHub**: a Rust step runs `git diff` against `GITHUB_BASE_REF` and | ||
| //! writes the bool result to `$GITHUB_ENV` under [`GH_ENV_IS_NON_PRODUCT`] | ||
| //! so it is accessible as a job-level output via | ||
| //! [`Pipeline::gh_job_id_of`] + `needs.<job>.outputs.is_non_product`. | ||
| //! | ||
| //! - **ADO**: an ADO step named [`ADO_STEP_NAME`] runs a bash script that | ||
| //! uses `git diff` against `SYSTEM_PULLREQUEST_TARGETBRANCH`, publishes the | ||
| //! result as an ADO output variable (`is_non_product`), and passes it to the | ||
| //! inline Rust snippet which writes it to the Flowey var. Downstream jobs | ||
| //! can gate on [`ado_condition`]. | ||
| //! | ||
| //! - **Local**: always writes `false` (conservative; vmm-tests always run). | ||
| //! |
There was a problem hiding this comment.
Please make it so that this is easier to maintain, like that this reads from a .toml file.
And, add unit tests for that parsing.
There was a problem hiding this comment.
Done. Bucket patterns now live in flowey/flowey_lib_hvlite/src/non_product_config.toml (embedded via include_str!, parsed with toml_edit). Added 10 unit tests covering: config parsing, Guide/**, repo_support/**/*.py, product code, workflow YAML, prefix-only matching, and prefix+suffix matching. (commit 128ecdc)
| // The ADO step must have a stable, well-known name so that | ||
| // downstream jobs can reference it as: | ||
| // dependencies.<job>.outputs['classify_pr_changes.is_non_product'] | ||
| // | ||
| // `emit_ado_step_with_inline_script` generates a step that: | ||
| // 1. Runs the bash classify script (sets IS_NON_PRODUCT + ADO output var). | ||
| // 2. Runs the Flowey inline snippet (writes the done signal). | ||
| // | ||
| // NOTE on format! escaping used in the YAML snippet below: | ||
| // In Rust format strings, `{{` and `}}` produce literal `{` and `}`. | ||
| // {{{{ ... }}}} → {{ ... }} (used for the {{FLOWEY_INLINE_SCRIPT}} marker) | ||
| // ${{VAR}} → ${VAR} (Rust escaping → bash variable expansion syntax) | ||
| ctx.emit_ado_step_with_inline_script("classify PR changes", |ctx| { | ||
| let done = done.claim(ctx); | ||
| ( | ||
| |_rt| { | ||
| format!( | ||
| concat!( | ||
| "- bash: |\n", | ||
| " set -euo pipefail\n", | ||
| " TARGET_BRANCH=\"${{SYSTEM_PULLREQUEST_TARGETBRANCH:-}}\"\n", | ||
| " if [[ -z \"$TARGET_BRANCH\" ]]; then\n", | ||
| " echo \"Not a PR run; treating as product change.\"\n", | ||
| " IS_NON_PRODUCT=false\n", | ||
| " else\n", | ||
| " TARGET_BRANCH=\"${{TARGET_BRANCH#refs/heads/}}\"\n", | ||
| " echo \"Comparing against: origin/$TARGET_BRANCH\"\n", | ||
| " CHANGED=$(git diff --name-only \"origin/$TARGET_BRANCH...HEAD\" 2>/dev/null || true)\n", | ||
| " if [[ -z \"$CHANGED\" ]]; then\n", | ||
| " echo \"No changed files found; treating as product change.\"\n", | ||
| " IS_NON_PRODUCT=false\n", | ||
| " else\n", | ||
| " IS_NON_PRODUCT=true\n", | ||
| " while IFS= read -r F; do\n", | ||
| " if [[ \"$F\" == Guide/* ]] || [[ \"$F\" =~ ^repo_support/.*\\.py$ ]]; then\n", | ||
| " : # file is in a non-product bucket\n", | ||
| " else\n", | ||
| " echo \"Product file detected: $F\"\n", | ||
| " IS_NON_PRODUCT=false\n", | ||
| " break\n", | ||
| " fi\n", | ||
| " done <<< \"$CHANGED\"\n", | ||
| " fi\n", | ||
| " fi\n", | ||
| " echo \"is_non_product=$IS_NON_PRODUCT\"\n", | ||
| " echo \"##vso[task.setvariable variable=is_non_product;isOutput=true]$IS_NON_PRODUCT\"\n", | ||
| " {{{{FLOWEY_INLINE_SCRIPT}}}}\n", | ||
| " name: {step_name}\n" | ||
| ), | ||
| step_name = ADO_STEP_NAME, |
There was a problem hiding this comment.
Yuck. Do we /really/ need this?
There was a problem hiding this comment.
Simplified significantly. The ADO emit_ado_step_with_inline_script is now just a minimal wrapper to set the step name: field (required for ADO cross-job output variable references). The bash template is now just {{FLOWEY_INLINE_SCRIPT}} — all classification logic lives in the same Rust classify() helper shared with the GitHub backend. No more duplicate bash classification script. (commit 128ecdc)
…ests Co-authored-by: mattkur <13772048+mattkur@users.noreply.github.com>
Every PR triggers all 6 expensive
vmm-testsjobs today, including PRs that only touch docs or repo scripts. This adds a reusable Flowey-level classification mechanism that skipsvmm-testsfor PRs entirely within approved non-product buckets, while keeping the full matrix for anything that touches product code or unclassified paths.The classification works across all three Flowey backends (GitHub, ADO, local) with no external scripts or GitHub API calls. The same Rust classification code runs on all backends — no backend-specific logic is duplicated.
Flowey node:
check_pr_changesBackend-native Rust node that classifies PR changed files using
git diff:git diff origin/$GITHUB_BASE_REF...HEAD$GITHUB_ENVasFLOWEY_IS_NON_PRODUCT; declared as a job-level outputgit diff origin/$SYSTEM_PULLREQUEST_TARGETBRANCH...HEAD##vso[task.setvariable;isOutput=true]; step nameclassify_pr_changesNon-product bucket config:
non_product_config.tomlBucket patterns are defined in
flowey/flowey_lib_hvlite/src/non_product_config.toml— a single TOML file that is the authoritative source for all backends. To add or remove a non-product bucket, edit this file only.The config is embedded at compile time via
include_str!and parsed withtoml_edit. Unit tests incheck_pr_changes.rscover config parsing, prefix-only matching, prefix+suffix matching, and product/non-product classification for representative paths.New Flowey infrastructure
New
PipelineJobandPipelinemethods:gh_set_job_output_from_env_var(name, env_var)— declares a GitHub job-level output backed by a$GITHUB_ENVvariable written by a Rust stepado_dangerous_override_if(condition)— overrides the ADO jobcondition:(mirrors the existinggh_dangerous_override_if)Pipeline::gh_job_id_of(handle)/Pipeline::ado_job_id_of(handle)— dynamically look up the auto-generated or override job ID from a handlePipelineJobCtx::new_var<T>()— creates typed(ReadVar<T>, WriteVar<T>)pairs for use indep_onclosuresAlso adds
outputs:support to theJobstruct in the GitHub YAML definitions.Pipeline changes (
checkin_gates.rs)For
PipelineConfig::Pron both GitHub and ADO backends, a newclassify PR changesjob is added. All 6run vmm-tests [...]jobs take a non-artifact dependency on it and gate on a backend-appropriate condition:GitHub:
ADO:
The classify job is included in
all_jobs, soopenvmm checkin gatesstill waits for it and the required-check model is preserved.Guide documentation
New section in
Guide/src/dev_guide/dev_tools/flowey/pipelines.mdcovering cross-job conditions, the new APIs (gh_job_id_of,gh_set_job_output_from_env_var,ado_dangerous_override_if), thecheck_pr_changesnode (including how to extend non-product buckets via the TOML config), and a pipeline usage example.Behavior matrix
Guide/**onlyrepo_support/**/*.pyonlyOriginal prompt
This section details on the original issue you should resolve
<issue_title>don't run vmm-tests for Guide changes</issue_title>
<issue_description># Skip
vmm-testsfor lightweight PRs without weakening PR gatesSummary
I want to stop burning
vmm-testscapacity on PRs that clearly are not product changes.The immediate case is
Guide/**, but I don’t think we should solve this as a docs-only special case. We should use this to add a small Flowey-level mechanism for classifying PRs into “product” vs “lightweight” buckets, then use that classification to skip the expensivevmm-testsjobs when the PR is lightweight-only.That keeps the current PR gate model intact, fixes the docs case, and gives us a clean path to exempt other non-product areas later, like
repo_support/**/*.py.What’s wrong today
Right now the main PR workflow is Flowey-generated from
.flowey.tomlviaci checkin-gates --config=pr, with the logic inflowey/flowey_hvlite/src/pipelines/checkin_gates.rs.That workflow always emits the full
vmm-testsmatrix, including the six heavyrun vmm-tests [...]jobs. That makes sense for product changes. It does not make sense for changes that only touch docs or other repo-maintenance surfaces.For example:
Guide/**changes already have their own docs validation pathrepo_support/relabel_backported.pyis repo automation, not product behaviorThose changes do not affect guest behavior, device behavior, save/restore behavior, boot behavior, or VM management semantics, but today they still consume the same scarce self-hosted test capacity as a real product change.
That is the real problem here. We are treating every PR like a product PR because we do not have a reusable way to say “this change is lightweight.”
Why I want to solve this more generally
I don’t want to hardcode one exception for
Guide/**and then repeat the same discussion the next time we want to exempt another obviously non-product path.The better model is:
vmm-testsvmm-testsThat makes the docs case easy, and it also gives us a clean way to handle future cases like
repo_support/**/*.pywithout inventing another one-off rule.The default should still be conservative: if a path is not explicitly classified as lightweight, it is treated as product-affecting.
Existing validation we already have
We already have a separate Flowey-generated docs PR workflow:
.github/workflows/openvmm-docs-pr.yaml.flowey.tomlviaci build-docs --config=prflowey/flowey_hvlite/src/pipelines/build_docs.rsThat workflow builds the guide via
flowey/flowey_lib_hvlite/src/build_guide.rs, which runs:mdbook testmdbook buildSo for
Guide/**, this is not about removing validation. It is about stopping redundant product validation for a change that already has an appropriate validation path.Proposed shape
I want to add a small reusable Flowey helper that classifies a PR into named change buckets before we expand the expensive jobs.
Conceptually, the classifier should answer things like:
Guide/**?repo_support/**/*.py?I’m fine with backing that helper using a purpose-built GitHub Action. In fact, that is probably the cleanest implementation, as long as the Flowey interface stays simple and the action contract stays small.
Then
checkin_gates.rscan use that classification to gate the sixvmm-testsjobs on!lightweight_only.So the behavior becomes:
Guide/**-only PR → skipvmm-testsrepo_support/**/*.py-only PR → skipvmm-testsGuide/**+ product code PR → runvmm-testsvmm-testsThat is the behavior I want.
Why I do not want to skip the whole PR workflow
The tempting shortcut here is to add workflow-level path filtering and just not run the main PR workflow for guide-only changes.
I don’t think that is the right shape.
The current PR pipeline already has an aggregate required-check job,
openvmm checkin gates, which depends on the rest of the pipeline and treats onlyfailureandcancelledas blocking. That is a good model. I want to preserve it.If we skip only the
vmm-testsjobs, we keep the existing required check surface and mixed-change behavior stays correct.If we skip the entire workflow, required-check behavior gets more fragile and the whole thing becomes harder to reason about.
So I want job-level skipping of the expensive matrix, not workflow disappearance.
Goals
vmm-testsfor PRs that are entirely within approved lightweight buckets.vmm-testsmatrix for anything that touches product ...💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.