test: e2e fixture improvements #238
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughSwitch Makefile includes to Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TopMake as "Top-level Makefile"
participant E2EMake as "e2e/fixtures/Makefile"
participant Vars as "e2e/fixtures/vars/*"
participant Ansible as "Ansible / Provisioner"
User->>TopMake: make deploy-lima-fixture FIXTURE_VARIANT=large
TopMake->>E2EMake: invoke _deploy-lima-fixture wrapper
E2EMake->>Vars: read vars/large.yaml and vars/lima.yaml (+ FIXTURE_EXTRA_VARS)
Vars-->>E2EMake: merged extra-vars (variant + provider + extras)
E2EMake->>Ansible: run playbook with merged extra-vars
Ansible->>Ansible: provision hosts, deploy control plane
Ansible-->>E2EMake: return playbook results + test-config
E2EMake-->>TopMake: return status / path to test-config
TopMake-->>User: fixture ready / test-config path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2026-01-08T16:29:19.608ZApplied to files:
📚 Learning: 2026-01-08T16:29:27.297ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (15)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @docs/development/e2e-tests.md:
- Around line 422-424: The docs contain a typo: change the referenced Make/Task
target name from "teardown-{lima,ec2}-target" to the correct
"teardown-{lima,ec2}-fixture" in the sentence that warns to run teardown before
switching variants; update the string literal in that line so it matches the
actual target names used elsewhere (i.e., replace "teardown-{lima,ec2}-target"
with "teardown-{lima,ec2}-fixture").
- Around line 410-420: The listed shell commands are missing the required make
prefix; update the examples so the targets are invoked via make by changing
deploy-lima-fixture, update-lima-fixture, reset-lima-fixture, and
teardown-lima-fixture to make deploy-lima-fixture, make update-lima-fixture,
make reset-lima-fixture, and make teardown-lima-fixture respectively so the
documented commands match the project's Makefile targets.
In @e2e/fixtures/vars/large_global.yaml:
- Around line 6-7: Host-2 is missing a region field; update the host entry for
"host-2" (the block containing swarm_manager: false) to include a region key
matching the other hosts (e.g., set region: us-west-1) so the host definitions
are consistent and avoid deployment issues.
🧹 Nitpick comments (2)
Makefile (1)
172-176: Minor: Trailing whitespace on line 174.There appears to be trailing whitespace after
_deploy-lima-fixture:. This is cosmetic but could be cleaned up.♻️ Optional cleanup
.PHONY: deploy-lima-fixture -deploy-lima-fixture: _deploy-lima-fixture - +deploy-lima-fixture: _deploy-lima-fixture +e2e/fixtures/Makefile (1)
15-18: Consider validating variant file existence.If an invalid
VARIANTis specified, Ansible will fail with a potentially confusing error about a missing vars file. You might consider adding validation or documenting valid variants more prominently.This is optional since Ansible's error message will indicate the missing file path, which should be sufficient for debugging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.circleci/config.ymlMakefileapi/Makefilecommon.mkdocs/development/e2e-tests.mde2e/fixtures/Makefilee2e/fixtures/vars/ec2.yamle2e/fixtures/vars/huge.yamle2e/fixtures/vars/huge_global.yamle2e/fixtures/vars/large.yamle2e/fixtures/vars/large_global.yamle2e/fixtures/vars/lima.yamle2e/fixtures/vars/lima_rocky_global.yamle2e/fixtures/vars/small.yamle2e/fixtures/vars/small_global.yaml
💤 Files with no reviewable changes (3)
- e2e/fixtures/vars/small.yaml
- e2e/fixtures/vars/lima_rocky_global.yaml
- e2e/fixtures/vars/lima.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (22)
e2e/fixtures/vars/huge.yaml (2)
8-20: Swarm manager redistribution maintains correct quorum.The swarm_manager flag changes redistribute the 5 manager nodes from hosts 1,2,3,4,5 to hosts 1,2,5,6,9. This maintains the odd number (5) required for Raft consensus quorum in Docker Swarm.
1-26: No action required. The refactoring correctly relocates provider-specific metadata to dedicated files: ec2.yaml containsminio_enabled,provider,architecture, andinstance_os, while lima.yaml containsminio_enabled,provider,architecture, and provider-specific s3 configuration. The variant file (huge.yaml) appropriately contains only the machines topology.e2e/fixtures/vars/small_global.yaml (1)
1-11: LGTM! Valid minimal multi-region cluster configuration.The configuration defines a minimal viable cluster with 3 manager nodes distributed across 3 different AWS regions (us-west-1, af-south-1, ap-southeast-4). This provides both quorum (odd number) and geographic distribution for global testing scenarios.
e2e/fixtures/vars/large.yaml (1)
6-12: Swarm manager redistribution maintains valid quorum.The changes redistribute the 3 manager nodes from hosts 1,2,3 to hosts 1,3,5. This maintains the required odd number for quorum and provides more even spacing across the host range.
e2e/fixtures/vars/huge_global.yaml (2)
10-29: Review swarm manager distribution across regions.The 5 swarm managers are distributed unevenly across regions:
- us-west-1: 2 managers (hosts 1, 2)
- af-south-1: 2 managers (hosts 5, 6)
- ap-southeast-4: 1 manager (host 9)
For optimal fault tolerance in a multi-region setup, consider a more balanced distribution (e.g., 2-2-1 could leave ap-southeast-4 vulnerable if both other regions fail). However, if this distribution is intentional based on expected load or network topology, this is acceptable.
1-38: The review comment is based on an incorrect premise. Thes3_repositoryblock was never present inhuge_global.yaml. Git history shows this file has only one commit (when it was added), and the current content contains only the machines configuration. Thes3_repositoryconfiguration exists in the separatelima.yamlfixture file (for the Lima provider), not inhuge_global.yaml.Likely an incorrect or invalid review comment.
e2e/fixtures/vars/large_global.yaml (2)
3-19: Well-balanced manager distribution across regions.The 3 swarm managers are distributed evenly with one manager per region (us-west-1, af-south-1, ap-southeast-4). This provides optimal fault tolerance for a multi-region deployment, as the cluster can tolerate the loss of any single region.
1-19: Verify removed configurations are provided elsewhere.Similar to other files in this refactor, top-level keys (minio_enabled, provider, architecture, template) and the s3_repository block were removed. Ensure these configurations are now provided via provider-specific files, environment variables, or FIXTURE_EXTRA_VARS to maintain functionality.
api/Makefile (1)
1-1: LGTM!The include path update aligns with the migration to
common.mkacross the repository..circleci/config.yml (1)
17-17: LGTM!Updating the checksum to include
common.mkensures the cache is invalidated when tool versions change. This is consistent with the migration fromtools.mk.Makefile (6)
1-1: LGTM!Consistent with the
common.mkmigration.
9-9: LGTM!Increasing default parallelism to 8 should improve test execution time.
14-16: LGTM!Clear variable definitions with sensible defaults. The
largedefault variant aligns with the Docker Compose setup.
33-40: LGTM!The 45-minute timeout accommodates larger fixture variants and higher parallelism.
158-170: Clean pattern-based approach for fixture orchestration.The logic correctly:
- Prepends
external_control_plane_imagewhenFIXTURE_CONTROL_PLANE_IMAGEis set- Skips
goreleaser-buildwhen an external image is specifiedThe
%pattern target elegantly handles bothlimaandec2variants with a single definition.
178-230: LGTM!The fixture lifecycle targets (
update,reset,stop,teardown) follow a consistent pattern. The separation between internal pattern targets and public wrappers provides flexibility while maintaining a clean public API.e2e/fixtures/vars/ec2.yaml (1)
1-5: LGTM!Clean separation of provider-specific configuration (EC2, x86_64, Rocky 9) from variant-specific configuration (machine counts). This enables reusing variant definitions across Lima and EC2.
e2e/fixtures/Makefile (2)
1-9: LGTM!The
VARIANTmechanism cleanly separates provider configuration (lima.yaml, ec2.yaml) from variant configuration (small.yaml, large.yaml, huge.yaml). The--extra-varsordering ensures proper precedence: provider defaults → variant settings → explicit overrides.
55-58: LGTM!Consistent with the Lima targets - the EC2 playbook invocation follows the same pattern for variant vars inclusion.
docs/development/e2e-tests.md (2)
211-226: LGTM!Clear documentation of the fixture lifecycle targets with helpful context about VM creation.
427-439: LGTM!Good documentation of available variants and the global variants with simulated latency. This provides clear guidance for test configuration selection.
common.mk (1)
12-16: Deferred expansion pattern is correctly implemented.The idiom ensures
CONTROL_PLANE_VERSIONis computed lazily on first access and cached thereafter. The?=allows external override, and the$(eval ...)defers evaluation until the variable is first referenced.Note on network dependency: The
git fetch --quietrequires network access and runs whenCONTROL_PLANE_VERSIONis first accessed. This is acceptable for the current usage context (build, deployment, and E2E fixture targets), but could fail in offline scenarios. If offline builds become a requirement, consider making git fetch optional or caching the version.
jason-lynch
force-pushed
the
test/e2e-makefile-improvements
branch
2 times, most recently
from
016d10c to
653a48d
Compare
jason-lynch
force-pushed
the
test/e2e-makefile-improvements
branch
from
653a48d to
887568d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @docs/development/e2e-tests.md:
- Around line 410-411: The example command is missing the required make prefix;
update the line containing deploy-lima-fixture with the FIXTURE_VARIANT=small
argument to prepend "make" so it reads make deploy-lima-fixture
FIXTURE_VARIANT=small, keeping formatting consistent with other examples in the
docs.
🧹 Nitpick comments (2)
e2e/fixtures/Makefile (1)
6-10: Add validation or documentation for missing variant files.The
variant_vars_fileis constructed from theVARIANTvariable without validation. If a user provides an invalid variant name, Ansible will fail with a potentially confusing error when it tries to load the non-existent vars file.Consider adding a comment or validation
Add a comment documenting the available variants, or consider adding validation in the targets that use this variable:
VARIANT ?= large EXTRA_VARS ?= +# variant_vars_file is constructed from VARIANT (available: small, large, huge, small_global, large_global, huge_global) variant_vars_file=vars/$(VARIANT).yamlAlternatively, you could add validation to targets that depend on this variable, though that may be overkill given the targets are invoked through documented top-level Make targets.
docs/development/e2e-tests.md (1)
422-424: Clarify consequences of not tearing down before switching variants.The note warns about running teardown before switching variants but doesn't explain what happens if you don't. Users might wonder if it's just a best practice or if it will cause errors.
Consider adding clarification
> [!NOTE] -> Make sure to run the `teardown-{lima,ec2}-fixture` before switching variants. +> Make sure to run `teardown-{lima,ec2}-fixture` before switching variants, as deploying a different variant without teardown may leave orphaned VMs or cause deployment conflicts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.circleci/config.ymlMakefileapi/Makefilecommon.mkdocs/development/e2e-tests.mde2e/fixtures/Makefilee2e/fixtures/vars/ec2.yamle2e/fixtures/vars/huge.yamle2e/fixtures/vars/huge_global.yamle2e/fixtures/vars/large.yamle2e/fixtures/vars/large_global.yamle2e/fixtures/vars/lima.yamle2e/fixtures/vars/lima_rocky_global.yamle2e/fixtures/vars/small.yamle2e/fixtures/vars/small_global.yaml
💤 Files with no reviewable changes (3)
- e2e/fixtures/vars/small.yaml
- e2e/fixtures/vars/lima_rocky_global.yaml
- e2e/fixtures/vars/lima.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- common.mk
- e2e/fixtures/vars/small_global.yaml
- .circleci/config.yml
- e2e/fixtures/vars/ec2.yaml
- e2e/fixtures/vars/large.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-08T16:29:19.608Z
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 238
File: e2e/fixtures/vars/huge_global.yaml:5-38
Timestamp: 2026-01-08T16:29:19.608Z
Learning: In e2e/fixtures/vars/*.yaml, treat the 'region' field as a spec for simulated latency in tests, not as the real deployment region. Ensure test latency computation uses this value and that reviewers understand it does not affect infrastructure placement. Consider adding a brief note in fixture docs clarifying this distinction.
Applied to files:
e2e/fixtures/vars/huge_global.yamle2e/fixtures/vars/huge.yamle2e/fixtures/vars/large_global.yaml
📚 Learning: 2026-01-08T16:29:19.608Z
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 238
File: e2e/fixtures/vars/huge_global.yaml:5-38
Timestamp: 2026-01-08T16:29:19.608Z
Learning: In the E2E fixture configuration files (e2e/fixtures/vars/*.yaml), the `region` field specified for each host machine is used to compute simulated latency for testing purposes, not to determine the actual deployment region for the infrastructure.
Applied to files:
docs/development/e2e-tests.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
e2e/fixtures/vars/huge.yaml (1)
2-26: LGTM! Swarm manager distribution maintains proper quorum.The swarm manager role changes result in 5 managers (host-1, host-2, host-5, host-6, host-9) across the 12-node cluster, which maintains an odd number for proper quorum and can tolerate up to 2 manager failures.
e2e/fixtures/vars/huge_global.yaml (1)
2-38: LGTM! Swarm configuration and region distribution look correct.The changes maintain proper swarm manager quorum (5 managers) and add region fields to support multi-region fixture testing. The region field is used for simulated latency in tests rather than actual infrastructure placement, with a balanced distribution across us-west-1, af-south-1, and ap-southeast-4 (4 hosts each).
Based on learnings, the region field drives test latency simulation rather than deployment location.
Makefile (3)
1-1: LGTM! Centralized configuration through common.mk.The switch to
common.mkaligns with the repository-wide refactoring and centralizes shared variable definitions (includingCONTROL_PLANE_VERSION).
9-9: Note the E2E test configuration changes.The default parallel execution has been set to 8 workers, and the timeout has been significantly increased from 20m to 45m. The timeout increase likely accommodates the expanded fixture deployment workflows.
Also applies to: 33-33
154-230: LGTM! Well-structured fixture lifecycle targets.The new fixture orchestration framework provides a clean pattern-based approach for managing fixture lifecycles (deploy/update/reset/stop/teardown) across Lima and EC2 providers. The conditional build logic correctly skips
goreleaser-buildwhen an external Control Plane image is specified, and the variable passing to sub-makes properly propagates variant and extra vars configurations.e2e/fixtures/vars/large_global.yaml (1)
2-20: LGTM! Swarm quorum and multi-region distribution are correct.The changes maintain proper swarm manager quorum (3 managers: host-1, host-3, host-5) for the 6-node cluster and add balanced region distribution (2 hosts per region) for multi-region testing. The removal of the
s3_repositoryblock is consistent with the refactoring seen inhuge_global.yaml.Based on learnings, the region field drives test latency simulation rather than deployment location.
api/Makefile (1)
1-1: No verification needed — common.mk correctly provides required tools.Verification confirms that
common.mk(at the repository root) defines bothgoa(line 6) andyamlfmt(line 9), which are used in api/Makefile'sgenerate-v1target (lines 5-6). The include path../common.mkis correct.e2e/fixtures/Makefile (2)
15-18: LGTM! Ansible extra-vars ordering enables proper override behavior.The
variant_vars_fileis loaded between the provider-specific vars file andEXTRA_VARS, which allows users to override variant settings when needed. This is the correct ordering for Ansible's variable precedence.
55-58: LGTM! Consistent with Lima configuration.The EC2 ansible playbook invocation mirrors the Lima configuration structure, maintaining consistency across providers.
docs/development/e2e-tests.md (4)
210-226: LGTM! Clear documentation of Lima fixture lifecycle.The updated targets provide a cleaner, more intuitive workflow compared to invoking individual playbook targets. The descriptions clearly explain what each target does.
228-235: LGTM! Accurate description of test config generation.The documentation correctly describes which targets generate the test config file and provides a helpful example of viewing it.
257-268: LGTM! Consistent use of FIXTURE_EXTRA_VARS.The updated examples correctly demonstrate passing architecture overrides using
FIXTURE_EXTRA_VARSacross all relevant targets.
433-440: Helpful context on global variants with simulated latency.The description of global variants aligns with the retrieved learning that the
regionfield is used for simulated latency computation rather than actual deployment region. This is clear and informative.Based on learnings, the region field in variant configurations is correctly documented as being for simulated latency purposes.
jason-lynch
force-pushed
the
test/e2e-makefile-improvements
branch
from
887568d to
084ebf0
Compare
We changed our release tagging scheme so that release tags are created on the release branches instead of on `main`. This change broke the `CONTROL_PLANE_VERSION` computation on `main`, resulting in it always returning an older version. This changes the `CONTROL_PLANE_VERSION` computation check for the latest release tag on the current release branch. This requires a git fetch to run reliably, so I've used a "deferred simple variable expression" idiom to ensure we only compute this value once when it's used.
Refactors the e2e fixture vars files to reuse the `machines` configuration for both the Lima and EC2 fixtures. This change also simplifies the mechanism to specify different fixture variants and changes the default variant to `large` to match the Docker compose-based setup.
Adds top-level make targets for managing E2E fixtures to cover common
use cases and reduce the number of commands to use the fixtures:
- `deploy-{lima,ec2}-fixture`: Builds the Control Plane binaries,
deploys and sets up the VMs, then deploys the Control Plane.
- `update-{lima,ec2}-fixture`: Builds and deploys the Control Plane to
an already-deployed fixture.
- `reset-{lima,ec2}-fixture`: Builds the Control Plane binaries, tears
down the existing Control Plane installation, and redeploys the
Control Plane.
- `stop-{lima,ec2}-fixture`: Stops the VMs
- `teardown-{lima,ec2}-fixture`: Deletes the VMs
jason-lynch
force-pushed
the
test/e2e-makefile-improvements
branch
from
084ebf0 to
3f28466
Compare
Summary
Makes a few improvements to our image build, E2E fixtures, and make targets to cover common use cases and reduce the number of commands needed to run them.
Changes
e2e/fixturesdirectory. Now you only need the variant name, such ashuge.deploy-{lima,ec2}-fixture: Builds the Control Plane binaries, deploys and sets up the VMs, then deploys the Control Plane.update-{lima,ec2}-fixture: Builds and deploys the Control Plane to an already-deployed fixture.reset-{lima,ec2}-fixture: Builds the Control Plane binaries, tears down the existing Control Plane installation, and redeploys the Control Plane.stop-{lima,ec2}-fixture: Stops the VMsteardown-{lima,ec2}-fixture: Deletes the VMsSee the documentation diff for usage updates.
Testing
Follow the updated instructions in the
e2e-tests.mddoc.Summary by CodeRabbit
Chores
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.