Skip to content

feat(preflight): add plugin, API version, and image checks before deploy - #21

Open
madhugoutham wants to merge 4 commits into
aneeshkp:mainfrom
madhugoutham:feat/preflight-compatibility
Open

feat(preflight): add plugin, API version, and image checks before deploy#21
madhugoutham wants to merge 4 commits into
aneeshkp:mainfrom
madhugoutham:feat/preflight-compatibility

Conversation

@madhugoutham

Copy link
Copy Markdown

What

  • Add test_00_preflight phase that runs before deploying any manifest
  • Three checks: EPP plugin compatibility, CRD API version, image existence
  • Catches manifest/cluster mismatches instantly instead of waiting 10+ minutes for MinimumReplicasUnavailable

Testing

  • 55 unit tests pass (uv run pytest tests/)
  • Smoke tested against real conformance manifests (main + 3.4-stable branches)

Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
@aneeshkp

Copy link
Copy Markdown
Owner

Code Review — Automated Analysis

7 findings from automated review. 4 are blocking.


🔴 Blocking

1. Dead fallback branch in resolve_available_pluginssrc/conformance/preflight.py:170

if not epp_tag: is unreachable when the tag exists in the cluster but is absent from the locally-cloned router repo. In that case the exact-tag git grep returns empty, the function returns (None, 'no detection method available'), and the plugin compatibility check is silently skipped every run.

# current — unreachable when epp_tag is set but tag missing from local repo
if not epp_tag:
    ...

# fix
elif not epp_tag:
    ...

2. AttributeError crash on explicit null in manifest YAMLsrc/conformance/preflight.py:44

If a manifest has spec.router.scheduler: null (explicit null, not a missing key), the .get() chain uses the default {} only for missing keys — not for None values. The next .get('config', {}) raises AttributeError: 'NoneType' object has no attribute 'get'. The phase becomes a pytest ERROR instead of a skip, violating the module's "skip safely, never false-fail" contract.

# fix — treat None and missing the same way
scheduler = (spec.get("router") or {}).get("scheduler") or {}

3. router_repo attribute never set — Layer-2 source detection permanently disabledtests/test_conformance.py:127

getattr(deployer, 'router_repo', None) always returns None because neither Deployer.__init__ nor conftest.py ever assigns router_repo. The entire fallback described in the module docstring is unreachable from any test invocation.


4. test_00_preflight ignores test_mode — false failures in discover modetests/test_conformance.py:100

Every other phase calls _require_deployed() or does an early if test_mode == 'discover': pytest.skip(). test_00_preflight does neither. In --mode discover, it can fail on API version mismatches against an otherwise-valid deployment the user pointed --endpoint at.


🟡 Recommended

5. Fallback git grep pattern too broad — inserts false positivessrc/conformance/preflight.py:103

The fallback Type\s*=\s*" matches Go struct fields, HTTP content-type assignments, and test fixtures — not just EPP plugin type registrations. Strings like "json", "grpc", "v1" can land in the available-plugins set and pass compatibility checks they should fail.


6. probe_epp_plugins extracts non-plugin tokens from log linessrc/conformance/preflight.py:74

The regex r'"([a-z][a-z0-9-]+)"' matches every quoted lowercase string on a matching line, not just the plugin name. A line like Registered plugin "flow-control-dispatcher" using handler "round-robin" adds both strings to the set. round-robin is not in non_plugins so it enters available_plugins as a false positive.


7. test_older_has_fewer is a brittle assertiontests/test_preflight.py:289

Asserts the oldest git tag always has strictly fewer plugins than newest. Fails legitimately if any plugin is removed or renamed between versions, causing spurious CI failures unrelated to any code bug.


# Severity Location Issue
1 🔴 Blocking preflight.py:170 Dead fallback: ifelif
2 🔴 Blocking preflight.py:44 AttributeError on explicit null YAML node
3 🔴 Blocking test_conformance.py:127 router_repo never set, Layer-2 detection dead
4 🔴 Blocking test_conformance.py:100 test_00_preflight ignores test_mode
5 🟡 Recommended preflight.py:103 Broad git-grep pattern inserts false positives
6 🟡 Recommended preflight.py:74 Regex grabs handler names as plugin names
7 🟡 Recommended test_preflight.py:289 Brittle len() assertion breaks on plugin removal

Null-safe YAML walks, wire --router-repo, latest-tag only when cluster
tag is absent locally, and skip image checks only in discover mode.

Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
Null-safe YAML, wire --router-repo, fail-closed Layer-2 fallthrough,
narrow discover image skip, tighten PluginType/log extractors, soften
brittle tag-count test.

Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
@madhugoutham

Copy link
Copy Markdown
Author

Thanks for the review. Went through all seven notes.

  1. Fallback bug: real. Did not use the elif change (it would not fix it). We only fall back to latest when the cluster tag is missing from the local repo. If the tag is there but extract is empty, we skip instead of guessing.
  2. Null YAML crash: fixed.
  3. router_repo never wired: fixed. Pass --router-repo if you want Layer-2 from source.
  4. Discover mode: we only skip the image check. API and plugin checks still run.
  5. Broad git grep: tightened to PluginType assignments only.
  6. Log regex: only the plugin name after "registered plugin" / "plugin type", not other quoted words.
  7. Brittle older-vs-newer count test: softened so plugin removals do not flake CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants