Proposal: Separate cloud-image-val utility from test suites#570
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- Consider explicitly calling out a deprecation/migration timeline in the ADR (e.g., when the old test_suite integration is considered unsupported) so downstream teams can plan concrete milestones against this proposal.
- The ADR currently hardcodes
/tmp/civ-*paths in multiple sections; it might be worth introducing a single configuration/contract section that defines these paths (and whether they are configurable) to avoid divergence between documentation and implementation later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider explicitly calling out a deprecation/migration timeline in the ADR (e.g., when the old test_suite integration is considered unsupported) so downstream teams can plan concrete milestones against this proposal.
- The ADR currently hardcodes `/tmp/civ-*` paths in multiple sections; it might be worth introducing a single configuration/contract section that defines these paths (and whether they are configurable) to avoid divergence between documentation and implementation later.
## Individual Comments
### Comment 1
<location path="docs/ADR-001-separation-of-concerns.md" line_range="12" />
<code_context>
+
+This ADR proposes refactoring cloud-image-val (CIV) from a monolithic Python test framework into a minimal CLI tool that provisions cloud instances, executes arbitrary test commands,
+and aggregates results into standardized reports. This approach enables downstream teams to maintain tests in their own repositories using any testing framework (pytest, shell scripts, Go tests, etc.)
+while eliminating tight coupling and reducing CIVs maintenance burden.
+
+### Key Decisions
</code_context>
<issue_to_address>
**issue (typo):** Use the possessive form "CIV's" instead of "CIVs".
Please update the phrase to "reducing CIV's maintenance burden."
```suggestion
while eliminating tight coupling and reducing CIV's maintenance burden.
```
</issue_to_address>
### Comment 2
<location path="docs/ADR-001-separation-of-concerns.md" line_range="87" />
<code_context>
+
+Refactor CIV into a minimal infrastructure-only CLI tool that provisions cloud instances, executes arbitrary test commands, and aggregates results into standardized reports.
+
+**CIVs responsibilities:**
+- Manage infrastructure lifecycle (provision → expose metadata → cleanup)
+- Execute arbitrary test commands
</code_context>
<issue_to_address>
**issue (typo):** Change "CIVs" to the possessive "CIV's".
Please update this heading to "**CIV's responsibilities:**" to use the correct possessive form.
```suggestion
**CIV's responsibilities:**
```
</issue_to_address>
### Comment 3
<location path="docs/ADR-001-separation-of-concerns.md" line_range="93" />
<code_context>
+- Aggregate and validate test results into standardized format (JUnit XML)
+- Provide consistent exit codes and reporting for CI/CD pipelines
+
+**Test teams responsibilities:**
+- Write tests using any framework/language
+- Consume standardized instance metadata
</code_context>
<issue_to_address>
**suggestion (typo):** Use a possessive form for "Test teams".
Consider "Test teams' responsibilities:" or "Test team responsibilities:" depending on whether you mean multiple teams.
```suggestion
**Test teams' responsibilities:**
```
</issue_to_address>
### Comment 4
<location path="docs/ADR-001-separation-of-concerns.md" line_range="67-68" />
<code_context>
+
+2. Improve Maintainability
+- GitLab pipelines too complex for package testing needs
+- Too much time spend fixing CIV bugs vs too little time spend developing CIV
+- PR pipeline state mostly ignored by devs
+
</code_context>
<issue_to_address>
**issue (typo):** Correct "spend" to "spent" in this sentence.
Consider rewriting this bullet as: "Too much time spent fixing CIV bugs vs too little time spent developing CIV."
```suggestion
2. Improve Maintainability
- Too much time spent fixing CIV bugs vs too little time spent developing CIV
```
</issue_to_address>
### Comment 5
<location path="docs/ADR-001-separation-of-concerns.md" line_range="414" />
<code_context>
+
+### CIV Helper Libraries
+
+To make result reporting easy for all test frameworks, CIV needs to provides helper libraries for languages that do not provide native JUnit output or auto conversion.
+Alternative: CIV includes converters for common test output formats, e.g. Golang.
+
</code_context>
<issue_to_address>
**issue (typo):** Fix the verb form in "needs to provides helper libraries".
Consider rephrasing to: "CIV needs to provide helper libraries for languages that do not provide native JUnit output or auto conversion."
```suggestion
To make result reporting easy for all test frameworks, CIV needs to provide helper libraries for languages that do not provide native JUnit output or auto conversion.
```
</issue_to_address>
### Comment 6
<location path="docs/ADR-001-separation-of-concerns.md" line_range="559" />
<code_context>
+
+1. **CIV Core Simplification**: Core codebase reduced. Structure needs to guide developers.
+2. **Language Flexibility**: At least one spike using non-Python tests (shell or Go)
+3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, packages maintainers)
+4. **Performance**: Infrastructure provision time unchanged or faster (remove pytest overhead)
+5. **Security**: No bandit high-severity findings
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify "packages maintainers" phrasing.
"Packages maintainers" reads a bit awkwardly. Consider "package maintainers" or "packages team maintainers" instead.
```suggestion
3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, package maintainers)
```
</issue_to_address>
### Comment 7
<location path="docs/ADR-001-separation-of-concerns.md" line_range="613" />
<code_context>
+- `lib/aws_lib.py` (test-specific)
+- Duplicate `ci/aws.sh` (after osbuild migration)
+
+### New Test Suire Repositories
+- `tests/generic/test_generic.py` (from test_suite/generic/)
+- `tests/cloud/test_aws.py` (from test_suite/cloud/)
</code_context>
<issue_to_address>
**issue (typo):** Typo in "Suire"; should be "Suite".
Please update the heading to `### New Test Suite Repositories`.
```suggestion
### New Test Suite Repositories
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| This ADR proposes refactoring cloud-image-val (CIV) from a monolithic Python test framework into a minimal CLI tool that provisions cloud instances, executes arbitrary test commands, | ||
| and aggregates results into standardized reports. This approach enables downstream teams to maintain tests in their own repositories using any testing framework (pytest, shell scripts, Go tests, etc.) | ||
| while eliminating tight coupling and reducing CIVs maintenance burden. |
There was a problem hiding this comment.
issue (typo): Use the possessive form "CIV's" instead of "CIVs".
Please update the phrase to "reducing CIV's maintenance burden."
| while eliminating tight coupling and reducing CIVs maintenance burden. | |
| while eliminating tight coupling and reducing CIV's maintenance burden. |
There was a problem hiding this comment.
Fair point!
Since this is an ADR and may get a lot of eyeballs, I’d keep the nit-picky fix — or, if you prefer to avoid apostrophes with acronyms, we could also rephrase slightly:
while eliminating tight coupling and reducing CIV maintenance burden.Either way keeps the sentence clear and grammatically clean; feel free to pick whichever matches the docs style guide better.
|
|
||
| Refactor CIV into a minimal infrastructure-only CLI tool that provisions cloud instances, executes arbitrary test commands, and aggregates results into standardized reports. | ||
|
|
||
| **CIVs responsibilities:** |
There was a problem hiding this comment.
issue (typo): Change "CIVs" to the possessive "CIV's".
Please update this heading to "CIV's responsibilities:" to use the correct possessive form.
| **CIVs responsibilities:** | |
| **CIV's responsibilities:** |
| - Aggregate and validate test results into standardized format (JUnit XML) | ||
| - Provide consistent exit codes and reporting for CI/CD pipelines | ||
|
|
||
| **Test teams responsibilities:** |
There was a problem hiding this comment.
suggestion (typo): Use a possessive form for "Test teams".
Consider "Test teams' responsibilities:" or "Test team responsibilities:" depending on whether you mean multiple teams.
| **Test teams responsibilities:** | |
| **Test teams' responsibilities:** |
| 2. Improve Maintainability | ||
| - GitLab pipelines too complex for package testing needs |
There was a problem hiding this comment.
issue (typo): Correct "spend" to "spent" in this sentence.
Consider rewriting this bullet as: "Too much time spent fixing CIV bugs vs too little time spent developing CIV."
| 2. Improve Maintainability | |
| - GitLab pipelines too complex for package testing needs | |
| 2. Improve Maintainability | |
| - Too much time spent fixing CIV bugs vs too little time spent developing CIV |
|
|
||
| ### CIV Helper Libraries | ||
|
|
||
| To make result reporting easy for all test frameworks, CIV needs to provides helper libraries for languages that do not provide native JUnit output or auto conversion. |
There was a problem hiding this comment.
issue (typo): Fix the verb form in "needs to provides helper libraries".
Consider rephrasing to: "CIV needs to provide helper libraries for languages that do not provide native JUnit output or auto conversion."
| To make result reporting easy for all test frameworks, CIV needs to provides helper libraries for languages that do not provide native JUnit output or auto conversion. | |
| To make result reporting easy for all test frameworks, CIV needs to provide helper libraries for languages that do not provide native JUnit output or auto conversion. |
|
|
||
| 1. **CIV Core Simplification**: Core codebase reduced. Structure needs to guide developers. | ||
| 2. **Language Flexibility**: At least one spike using non-Python tests (shell or Go) | ||
| 3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, packages maintainers) |
There was a problem hiding this comment.
suggestion (typo): Clarify "packages maintainers" phrasing.
"Packages maintainers" reads a bit awkwardly. Consider "package maintainers" or "packages team maintainers" instead.
| 3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, packages maintainers) | |
| 3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, package maintainers) |
| - `lib/aws_lib.py` (test-specific) | ||
| - Duplicate `ci/aws.sh` (after osbuild migration) | ||
|
|
||
| ### New Test Suire Repositories |
There was a problem hiding this comment.
issue (typo): Typo in "Suire"; should be "Suite".
Please update the heading to ### New Test Suite Repositories.
| ### New Test Suire Repositories | |
| ### New Test Suite Repositories |
F-X64
force-pushed
the
adr-proposal-test-seperation
branch
from
81b852e to
c2e5385
Compare
| - Manual Terraform/OpenTofu version synchronization | ||
| - `.gitlab-ci-cloud-experience.yaml` hardcodes runner names requiring manual updates | ||
|
|
||
| **4. Prevents Independent Test Maintenance** |
There was a problem hiding this comment.
Nit: I think it's fair to call out that we have both a maintenance and an ownership problem. CIV is used across several orgs within RH and the tight coupling creates bottlenecks for moving quickly.
|
|
||
| **4. Prevents Independent Test Maintenance** | ||
| - Image definitions live in osbuild-composer repository | ||
| - Verification tests live in cloud-image-val repository |
There was a problem hiding this comment.
Suggestion: these two read a little oddly with the statement that they are the chicken-and-egg, but the point remains that we're coupling dependency and consumer concerns.
| - No dependency separation (`requirements.txt` mixes test frameworks with runtime dependencies) | ||
| - Security vulnerabilities (`os.system()` calls without sanitization) | ||
|
|
||
| ### Business Drivers |
There was a problem hiding this comment.
Suggestion: we should consider adding speed to market as a business driver. The mandate from leaders is pretty clear on the ramp up on CVEs and it makes sense that we'll introduce more image testing over time and the coupling is both a slowdown and a risk to stability.
| 2. Improve Maintainability | ||
| - GitLab pipelines too complex for package testing needs | ||
| - Too much time spend fixing CIV bugs vs too little time spend developing CIV | ||
| - PR pipeline state mostly ignored by devs |
There was a problem hiding this comment.
Non-blocking Question: is the pipeline ignored by test contributors or developers in general?
| %% ============================================================ | ||
| %% LAYER 2: Filesystem Contract (Shared State) | ||
| %% ============================================================ | ||
| subgraph CONTRACT["Filesystem Contract Layer<br/><i>Shared state between cloud-image-val and downstream tests</i>"] |
There was a problem hiding this comment.
Nit Question: will CIV define and expose this contract to consumers or is this something already exposed in the FS?
| - `[D]` = Downstream test repositories (spawned by executor.py) | ||
| - `[E]` = Optional helper utilities (junit-reporter.*) | ||
|
|
||
| **Decoupled Design:** |
There was a problem hiding this comment.
Praise: I really like the approach. It allows for team optionality.
|
|
||
| ### CIV Helper Libraries | ||
|
|
||
| To make result reporting easy for all test frameworks, CIV needs to provides helper libraries for languages that do not provide native JUnit output or auto conversion. |
There was a problem hiding this comment.
Non-blocking question: is there a generic standard for conversion or would CIV need to develop these for each language separately?
| - JIRA API integration | ||
| - HTML report generation | ||
| - Test result modification | ||
| - **Fix:** split into plugins |
There was a problem hiding this comment.
Non-blocking Question: perhaps too implementation-specific for now, but would these plugins live in the CIV repo or would they get kicked out to a utility library/
|
|
||
| 2. **Inconsistent Naming Conventions** | ||
| - Python: `AzureConfigBuilderV2` vs `aws_config_builder` | ||
| - **Fix:** Use flake8 naming consistently in CI (future cleanup) |
There was a problem hiding this comment.
Non-blocking, implementation question: would we run these as pre-commits or would they just live on CI?
| | Risk | Impact | Solution | | ||
| |------|--------|------------| | ||
| | Breaking existing CI pipelines | HIGH | upgrade in staging environment first | | ||
| | Invalid JUnit XML output | HIGH | Mandatory XSD validation before CIV returns. Clear error messages guide test authors | |
There was a problem hiding this comment.
Non-blocking, implementation question: what would the feedback loop look like for test authors? Could they run this on their local to quickly iterate or would pipelines be involved? (Sorry for the newbie question, I'm not familiar with the dev experience.)
| 1. **CIV Core Simplification**: Core codebase reduced. Structure needs to guide developers. | ||
| 2. **Language Flexibility**: At least one spike using non-Python tests (shell or Go) | ||
| 3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, packages maintainers) | ||
| 4. **Performance**: Infrastructure provision time unchanged or faster (remove pytest overhead) |
There was a problem hiding this comment.
Non-blocking question: do we have baseline metrics for this currently or do we need to capture them?
cmchase
left a comment
cmchase
left a comment
There was a problem hiding this comment.
I think this is great overall and I really appreciate the time and thought you put into the initial design, but there are a lot of technical details I think we'll need input on from teams that consume CIV.
| 3. **Downstream Adoption**: 2+ teams using new approach in production (osbuild, packages maintainers) | ||
| 4. **Performance**: Infrastructure provision time unchanged or faster (remove pytest overhead) | ||
| 5. **Security**: No bandit high-severity findings | ||
| 6. **Type Safety**: Aiming for 80% type hint coverage in `core/` and `ssh/` |
There was a problem hiding this comment.
Non-blocking Question: are we looking to warn and not error on initial linting checks?
|
|
||
| ## 9. Open Questions for Review | ||
|
|
||
| 1. Use `/tmp/civ-*.json` or configurable path? |
There was a problem hiding this comment.
Observation: FWIW, Sourcery called this out in a previous comment.
2 participants
Summary
Proposes refactoring cloud-image-val from a monolithic Python test framework into a
minimal CLI tool focused solely on infrastructure orchestration. Tests would be
maintained independently by downstream teams in their own repositories.
Problem
The current codebase tightly couples infrastructure code (multi-cloud provisioning,
test execution) with test suites (RHEL/Fedora validation tests). This creates:
Proposed Solution
Transform cloud-image-val into infrastructure-only:
This ADR documents the architectural vision, current pain points, proposed API contracts, and migration strategy.
Review Focus
Note: This is a proposal document (ADR-001) for discussion, not a finished guide.