test(e2e): diff-driven install selector select-install.sh - #3279
Conversation
Add hack/select-install.sh: given the E2E suites select-e2e.sh picked, emit the forward dependsOn closure over the PackageSource graph — the minimal set of packages to install for those suites. Companion to the reverse (test-selection) walk in select-e2e.sh; keeps the cozystack-engine edge since install needs it. Suite->source mapping follows the <suite>-application convention with a bare-<suite> fallback plus explicit cases for irregular names (kubernetes/vminstance/securitygroup) and core-platform features that have no separately-enabled package (serviceexposure). Includes a --validate mode (reachability + cycle check) and a 13-case unit suite auto-discovered by make unit-tests. Track A of #2847; tracked in #3278. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new utility script designed to optimize E2E testing by calculating the precise set of packages that must be installed for a given suite. By performing a forward dependency closure over the existing Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughAdds ChangesPackage source selector
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SuiteInput
participant select-install.sh
participant PackageSourceYAML
participant yq
SuiteInput->>select-install.sh: provide suite names
select-install.sh->>PackageSourceYAML: read source manifests
select-install.sh->>yq: extract nodes and dependsOn edges
yq-->>select-install.sh: return dependency graph
select-install.sh->>select-install.sh: map suites and expand closure
select-install.sh-->>SuiteInput: print sorted PackageSources
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility script hack/select-install.sh along with its unit tests in hack/select-install_test.bats. The script determines the minimal set of packages required for E2E test suites by computing the forward dependency closure of PackageSources, and provides a validation mode to check for dangling dependencies and cycles. Feedback on the tests suggests optimizing them by directly using the default production sources path instead of copying files to a temporary directory, which simplifies the test code and avoids unnecessary I/O.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| tmp=$(mktemp -d) | ||
| trap 'rm -rf "$tmp"' EXIT | ||
| cp -r packages/core/platform/sources "$tmp/sources" | ||
| output=$(hack/select-install.sh "postgres" "$tmp/sources") |
There was a problem hiding this comment.
Since hack/select-install.sh defaults to packages/core/platform/sources when the second argument is omitted, and this test only reads the production sources without modifying them, you can avoid creating a temporary directory and copying the sources. This simplifies the test and speeds up execution by avoiding unnecessary I/O.
This same optimization can be applied to tests 2 through 11 in this file.
output=$(hack/select-install.sh "postgres")
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the forward dependsOn closure under-selects for suites backed by system packages: securitygroup resolves to an install set that omits the component serving the very API the suite exercises, and the unknown-suite path fails open (exit 0, empty output) with no guard to catch the next such drift.
Business context: select-e2e.sh already picks which Chainsaw suites a diff must run; this adds the install-side companion that computes the minimal package set to provision for those suites, so E2E can stop standing up the full platform for every PR.
Blockers
B1: the securitygroup closure omits cozystack.cozystack-engine, which serves the API the suite exercises
File: hack/select-install.sh:70
Issue: select-install.sh securitygroup emits cozystack.gateway-api-crds cozystack.networking cozystack.securitygroup-controller. That set cannot run the suite.
Evidence:
hack/e2e-chainsaw/securitygroup/securitygroup.yamlapplieskind: SecurityGroup,apiVersion: sdn.cozystack.io/v1alpha1.- That group is served by an aggregated apiserver, not a CRD:
packages/system/cozystack-api/templates/apiservice.yamlregistersAPIService v1alpha1.sdn.cozystack.io(group: sdn.cozystack.io, backingservice: cozystack-api).packages/system/securitygroup-controller/ships no CRD and no APIService — only a ServiceAccount, a Deployment and RBAC. cozystack-apiis a component ofcozystack.cozystack-engine(packages/core/platform/sources/cozystack-engine.yaml).- The forward walk cannot reach it:
packages/core/platform/sources/securitygroup-controller.yamldeclaresdependsOn: [cozystack.networking]only. Thecozystack.cozystack-engineedge exists solely on*-applicationsources; system packages do not carry it. - The suite's
member-pod.yamladditionally relies on the lineage webhook (its own comment: "The lineage webhook skips it"), andlineage-controller-webhookis likewise acozystack-enginecomponent.
This contradicts the script's own stated contract (lines 24-29): the engine edge is kept because "for INSTALL it is a genuine prerequisite that must be up". For this suite the forward walk cannot put it there, because the edge does not exist in the graph.
Impact: provision exactly the emitted set and the suite's first apply fails — sdn.cozystack.io is unserved. The unit test at hack/select-install_test.bats:91 asserts only that cozystack.securitygroup-controller is present, so it green-lights the incomplete set.
Fix: the forward dependsOn closure is an install-ordering closure — sources/kuberture.yaml states this verbatim ("Install-order dependencies only") — not a statement of what a suite needs in order to run. Either seed cozystack.cozystack-engine unconditionally for any non-empty closure, or map suites that exercise an aggregated *.cozystack.io API to the engine explicitly alongside their controller.
B2: an unknown suite fails open, and --validate guards the graph rather than the mapping that actually drifts
File: hack/select-install.sh:138
Issue: a suite with no mapping warns on stderr and is skipped — exit 0, empty stdout. Nothing catches the next mapping gap.
Evidence:
select-e2e.shauto-discovers the suite universe (find hack/e2e-chainsaw -mindepth 2 -maxdepth 2 -name chainsaw-test.yaml), so it grows with no code change here.suite_to_source()is hand-maintained, and 3 of the 22 current suites (kubernetes-*,vminstance,securitygroup) already need explicit overrides — the<suite>-application/ bare-<suite>convention is demonstrably not sufficient for a new irregular name.- Adding a suite directory with no matching source reproduces it: stdout empty, exit
0, and onlyselect-install: warning: suite 'newthing' has no known PackageSource mapping; skippingon stderr. - Empty stdout is overloaded — "no suites", "all suites unmapped", and "core-platform-only suite" (
serviceexposure) are indistinguishable to a caller. --validatechecks danglingdependsOntargets and cycles. That graph lives next to the packages and cannot drift from the suite directories; the hand-written mapping is the only thing that can, and it is unguarded.- The sibling script fails safe in the mirror case.
select-e2e.sh's closing net: "a system source with no*-applicationdescendants would otherwise silently skip E2E. Fall back to full suite so a path inside the graph is never silently dropped." This script fails open into "install nothing", andhack/select-install_test.bats:74pins that as the contract.
Impact: once wired, a suite whose package was never installed either fails on a missing CRD or passes vacuously, and the only signal is a warning line in a CI log.
Fix: extend --validate to assert that every hack/e2e-chainsaw/*/ suite resolves — to a source or to -. That is the cheap CI guard the description claims, pointed at the part that actually moves. In closure mode, make an unmappable suite a hard error rather than a warning.
Non-blocking follow-ups
- The documented stdin form (
-, line 15) has no test. It works, but it is the shape a CI consumer would use (select-e2e.sh … | select-install.sh -). build_forward_deps(line 80) unionsdependsOnacross allspec.variants[]rather than the variant actually installed —cozystack.networkinghas 6 variants,noopdeclares none while the other five declaregateway-api-crds. It over-selects rather than under-selects, so it is safe, but it softens the "minimal set" claim and is worth a line in the header comment.- The description says the
securitygroupsuite "also stands up a Kubernetes app". It does not — the fixtures are plain Pods carrying lineage labels plus aSecurityGroupwhoseattachments/fromAppreferencePostgresandKubernetesby kind and name as selectors only. The real gap for that suite is the engine (B1). docs/agents/e2e-testing.md§7 documents the selection side and asks thathack/*.shtest helpers be read before they are changed; the install-side selector is not mentioned there. Reasonable to defer to the PR that wires it in, but it should land with that PR.
Checked and dismissed, for the record: the serviceexposure suite mapping to no package looked wrong because its Chainsaw test references metallb.io/v1beta1 IPAddressPool — but that is an error: (must-not-exist) assertion and the fixture deliberately selects the externalIPs backend, so no MetalLB install is implied. Worth confirming separately that Chainsaw's error: operation tolerates an unregistered kind before this gets wired in. Likewise, kuberture's closure omits the engine while the suite applies a cozystack.io/v1alpha1 Package — but Package is reconciled by the operator (internal/operator/package_reconciler.go), which is bootstrap machinery rather than an enableable package. --validate's exit-status handling is correct in both failure modes.
| kubernetes-latest|kubernetes-previous|kubernetes-oidc-system|kubernetes-oidc-customconfig) | ||
| echo cozystack.kubernetes-application ; return ;; | ||
| vminstance) echo cozystack.vm-instance-application ; return ;; | ||
| securitygroup) echo cozystack.securitygroup-controller ; return ;; |
There was a problem hiding this comment.
The forward closure from cozystack.securitygroup-controller cannot reach cozystack.cozystack-engine, but this suite needs it.
hack/e2e-chainsaw/securitygroup/securitygroup.yaml applies sdn.cozystack.io/v1alpha1 SecurityGroup. That group is served by the aggregated apiserver — packages/system/cozystack-api/templates/apiservice.yaml registers APIService v1alpha1.sdn.cozystack.io backed by service: cozystack-api — and packages/system/securitygroup-controller/ ships no CRD and no APIService of its own. cozystack-api is a component of cozystack.cozystack-engine.
sources/securitygroup-controller.yaml declares dependsOn: [cozystack.networking] only, so the walk stops short:
$ hack/select-install.sh securitygroup
cozystack.gateway-api-crds cozystack.networking cozystack.securitygroup-controller
Install exactly that and the suite's first apply fails — nothing serves sdn.cozystack.io. The member-pod.yaml fixture also depends on the lineage webhook, another cozystack-engine component.
The header (lines 24-29) already argues the engine is "a genuine prerequisite that must be up" for install. That holds here too; the graph just has no edge to express it, because only *-application sources carry the engine edge. Consider seeding cozystack.cozystack-engine for any non-empty closure.
There was a problem hiding this comment.
Fixed in 1f084cecc. The closure now seeds cozystack.cozystack-engine for any non-empty selection, so securitygroup resolves to cert-manager cozystack-engine gateway-api-crds keycloak keycloak-operator networking postgres-operator prometheus-operator-crds securitygroup-controller — the aggregated cozystack-api that serves sdn.cozystack.io is now included. As you noted, the forward dependsOn walk is install-ordering only and the engine edge lives solely on *-application sources, so system-package suites can't reach it via the walk; it's seeded explicitly instead, and a graph missing the engine is now a hard error rather than a silent under-selection.
| src="$(suite_to_source "$suite")" | ||
| case "$src" in | ||
| "-") continue ;; # core-platform feature — nothing separate to enable | ||
| "") echo "select-install: warning: suite '$suite' has no known PackageSource mapping; skipping" >&2 ; continue ;; |
There was a problem hiding this comment.
This is the false-negative path, and it fails open: warn on stderr, continue, exit 0, empty stdout. A caller cannot distinguish "install nothing because nothing is needed" from "install nothing because I could not map the suite".
The suite universe is auto-discovered by select-e2e.sh (find hack/e2e-chainsaw … -name chainsaw-test.yaml) while suite_to_source() is hand-maintained — and 3 of the 22 current suites already need explicit overrides, so a new irregular name landing here is the norm, not the exception.
The sibling script takes the opposite stance in the mirror case: "Fall back to full suite so a path inside the graph is never silently dropped." An unmappable suite here should be a hard error (or escalate to the full source set), not a warning that scrolls past in a CI log.
Relatedly, --validate above checks the PackageSource graph — which is maintained alongside the packages and cannot drift from the suite directories — but not this mapping, which is the only part that can. Asserting that every hack/e2e-chainsaw/*/ suite resolves would be the cheap CI guard the description promises.
There was a problem hiding this comment.
Fixed in 1f084cecc. Two parts: (1) closure mode — an unmapped suite is now a hard error (every bad suite reported in one run, no partial stdout), not warn-and-skip; (2) --validate now also checks the mapping — it asserts every hack/e2e-chainsaw/*/ suite (discovered the same way select-e2e.sh does) resolves to a real PackageSource or -, and fails when the suites dir is missing. So the hand-maintained mapping is guarded, which is the only part that can actually drift. Both are covered by new unit tests.
| cp -r packages/core/platform/sources "$tmp/sources" | ||
| output=$(hack/select-install.sh "securitygroup" "$tmp/sources") | ||
| echo "$output" | grep -wq cozystack.securitygroup-controller | ||
| } |
There was a problem hiding this comment.
This test asserts only that cozystack.securitygroup-controller lands in the closure, so it passes against an install set that cannot actually run the suite (see the comment on select-install.sh:70 — cozystack.cozystack-engine serves sdn.cozystack.io and is missing).
Asserting the full expected set here, rather than a single member, would have caught it.
There was a problem hiding this comment.
Fixed in 1f084cecc. The securitygroup test now asserts the engine is present (alongside the controller and its stable transitive prerequisites — cert-manager / networking / gateway-api-crds), so a set that can't serve sdn.cozystack.io fails. I kept it to the stable members rather than pinning the full 9-package set, to avoid coupling the securitygroup test to unrelated engine-dependency churn (keycloak / postgres-operator / …).
Address the review blockers on the diff-driven install selector: - B1: seed cozystack.cozystack-engine for any non-empty closure. Suites backed by a system package (e.g. securitygroup, which applies a sdn.cozystack.io resource served by the aggregated cozystack-api) carry no dependsOn edge to the engine, so the forward walk could never reach it and the emitted set could not run the suite. Hard-error if the engine source is absent from the graph. - B2: an unmapped suite is now a hard error in closure mode (every bad suite reported in one run, no partial output) instead of failing open. --validate additionally asserts every hack/e2e-chainsaw suite resolves to a real PackageSource or "-", and fails when the suites dir is missing. Also correct the inaccurate "securitygroup also stands up a Kubernetes app" comment (its fixtures reference Postgres/Kubernetes as selectors only), note the variant-union over-selection in build_forward_deps, drop the unnecessary source copies from the tests, and add coverage for stdin input, the engine seed, fail-closed mapping, the --validate suite guard, and the missing-suites-dir / missing-engine failure modes. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — both blockers from the previous review are resolved: the closure now seeds cozystack.cozystack-engine for any non-empty selection (so securitygroup reaches the aggregated sdn.cozystack.io server), and an unmapped suite is now a hard error with --validate guarding the suite→source mapping.
Resolved
- B1 (engine under-selection).
select-install.sh securitygroupnow emits the engine — verified against the real 97-source graph, a 9-source closure that includescozystack.cozystack-engine. A graph missing the engine is a hard error (select-install.sh:211-216). The regression test asserts the engine plus its stable prerequisites rather than the controller alone (select-install_test.bats:89-102). - B2 (fail-open + unguarded mapping). An unmapped suite is now a hard error that reports every bad suite and emits no partial stdout (
select-install.sh:185-199);--validateasserts every discoveredhack/e2e-chainsaw/*/suite resolves to a real source or-, and a missing suites dir is itself a failure (select-install.sh:152-171). All four--validatefailure modes and the fail-closed closure have tests.
Also addressed from the prior non-blocking notes: the stdin form now has a test, the all-variant dependsOn union is documented as an intentional over-select (select-install.sh:93-96), and the description no longer claims the securitygroup suite stands up a Kubernetes app.
Verified locally: all 17 unit tests pass, --validate passes on the real graph, postgres resolves to 9 of 97 sources as described.
Non-blocking follow-ups
docs/agents/e2e-testing.mdstill documents only the selection side; the install-side selector is worth a mention when it is wired into CI. Fine to land with that follow-up.
What this PR does
Adds
hack/select-install.sh— the install-side companion tohack/select-e2e.sh. Whereselect-e2e.shpicks which Chainsaw suites run for a diff (a reverse dependency walk),select-install.shpicks what must be installed for those suites: the forwarddependsOnclosure over thePackageSourcegraph.Given the suite names
select-e2e.shemits, it maps each back to its*-applicationPackageSourceand walksspec.variants[].dependsOnforward to the transitive closure — the minimal set of packages to enable instead of provisioning the fullisp-fullplatform. Apostgreschart change, for example, resolves to 9 of 97 sources.Design notes:
PackageSourcegraph inpackages/core/platform/sources/— no parallel dependency-declaration files to keep in sync.select-e2e.sh's reverse walk, the forward walk keeps thecozystack.cozystack-engineedge: for test selection it is noise (it would fan every app out to every other), but for install it is a genuine prerequisite (the app's*-rdHelmRelease needs the engine to register the ApplicationDefinition CRD). Because that edge lives only on*-applicationsources, a suite backed by a system package (e.g.securitygroup, whoseSecurityGroupinsdn.cozystack.iois served by the aggregatedcozystack-api— acozystack-enginecomponent) can't reach the engine via the walk, so the engine is seeded explicitly for any non-empty closure (and a graph missing it is a hard error).--validatemode checks the whole graph — everydependsOntarget resolves to a realPackageSource(reachability) and there are no dependency cycles — and the suite mapping: everyhack/e2e-chainsaw/*/suite resolves to a realPackageSourceor to-(a missing suites dir is itself a failure). The graph lives next to the packages and can't drift from the suite dirs; the hand-maintained mapping is the only part that can, so this is the cheap CI guard.<suite>-applicationconvention with a bare-<suite>fallback, plus explicit cases for the irregular names (kubernetes/vminstance/securitygroup) and for core-platform features that have no separately-enabled package (serviceexposure, whoseExposureClass/ServiceExposureCRDs live inpackages/core/platform). A suite may reference other app kinds by selector without standing them up (e.g.securitygroup'sSecurityGroupnamesPostgres/Kubernetesas selectors only) — only the primary package is mapped here; the forward closure and the seeded engine cover its prerequisites, and full multi-package modelling is left to thetest-minimalfollow-up. A suite with no known mapping is a hard error (fail closed), not a silent skip.Includes a 17-case unit suite (
hack/select-install_test.bats) covering single-app closure, transitivity, multi-suite union, stdin input, the kubernetes/irregular/core-platform suite mappings, thesecuritygroupengine-seed regression, empty input, fail-closed unmapped suites (all reported, no partial output), the missing-engine hard error, and the--validatefailure modes (dangling dep, cycle, unmapped suite dir, missing suites dir). It is auto-discovered bymake unit-tests(BATS_UNIT_FILESglobshack/*.bats).Verified end-to-end against real git diffs across six path types (app chart, shared operator, docs-only, per-suite Chainsaw edit, library, shared e2e helper): a
postgreschart change scopes the install to 9 of 97 sources; the full-suite fallback to 39 of 97.This is Track A of #2847 (diff-driven & scenario-based E2E); tracked in #3278. Consuming the output — a
test-minimalplatform variant/bundle plus wiring into thepull-requests.yamlinstall step — is a deliberate follow-up, since it depends on a platform-variant design decision.Screenshots
Not applicable — CI/tooling-only change.
Release note
Summary by CodeRabbit
New Features
Tests