-
Notifications
You must be signed in to change notification settings - Fork 13
[HYPERFLEET-59] The spike report for GCP validation adapter #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HYPERFLEET-59] The spike report for GCP validation adapter #32
Conversation
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Show resolved
Hide resolved
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
ciaranRoche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, just one thing
- This focuses on what CS does today for GCP, can we add a section on what validation will be done by HO, I want us to see what kind of duplication will be in terms of validation between Hyperfleet and Hypershift.
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
9ecf7c1 to
3be2790
Compare
ciaranRoche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome work. Just one question have you explored using a CRD for status reporting 🤔
I am thinking if we defined a CRD, the status reporter or even the validator itself can create a CR to record the data, which the adapter can watch. WDYT?
Something like:
- Adapter framework creates ValidationResult CR (phase: Pending)
- Adapter framework creates Job
- Job runs, reporter updates ValidationResult CR (phase: Passed/Failed)
- Adapter framework watches ValidationResult, sees completion
- Adapter framework proceeds or handles failure
Might be overkill for MVP if reading the job conditions is enough. But would be nice to see an alternative in this document and the trade off's between them
80a00f5 to
b7ccd6c
Compare
WalkthroughAdds a new spike report defining a phased GCP validation adapter implemented as a Kubernetes Job with a two-container sidecar (validator + status reporter), Workload Identity Federation authentication, validation flow (MVP and post‑MVP), Job manifest examples, RBAC/IAM notes, and acceptance criteria. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Operator as Operator
participant K8s as Kubernetes API
participant Job as Validation Job (Pod)
participant Validator as Validator Container
participant Reporter as Status Reporter Sidecar
participant GCP as Google Cloud APIs/IAM
rect rgb(230, 248, 255)
note over Operator,K8s: Operator creates Job manifest (MVP: Kubernetes Job)
Operator->>K8s: apply Job manifest
K8s->>Job: schedule Pod (validator + reporter)
end
rect rgb(255, 245, 230)
note over Validator,GCP: Validator authenticates via WIF and calls GCP
Job->>Validator: start validation
Validator->>GCP: WIF token exchange / API calls
alt success
GCP-->>Validator: positive responses
Validator->>Reporter: write success status to shared volume
else failure
GCP-->>Validator: error
Validator->>Reporter: write failure status to shared volume
end
Reporter->>K8s: optionally expose status artifact / exit
end
rect rgb(230, 255, 235)
note over Operator,K8s: Operator retrieves results
Operator->>K8s: fetch Job logs or status artifact
K8s-->>Operator: return validation report
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (2)
619-644: Missing Section 7 header.The document jumps from Section 6.3 (MVP vs Post-MVP decision) directly to Section 7.1 (MVP - Proof of Concept). Add a Section 7 header to properly organize the roadmap content.
--- a/hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md +++ b/hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md @@ -641,6 +641,10 @@ The `AdapterResult` CRD approach should be reconsidered when: --- +## 7. MVP vs Post-MVP Implementation Roadmap + +### 7.1 MVP - Proof of Concept
651-671: Standardize list indentation in MVP Tasks section.The task lists (lines 651-671) use 4-space indentation instead of the Markdown standard 2-space indentation. Consider fixing for consistency.
Adjust list indentation from 4 spaces to 2 spaces in the MVP Tasks, Success Criteria, and Deliverables sections (lines 651-671).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
[style] ~4-~4: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...ang@redhat.com Date: November 21, 2025 Status: Reviewing Reviewers...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~300-~300: Use a hyphen to join words.
Context: ... } } ``` --- ## 4. Validation Adapter Related Configurations ### 4.1 Kubernet...
(QB_NEW_EN_HYPHEN)
[grammar] ~537-~537: Ensure spelling is correct
Context: ... └──────────────────────┘ ``` ### 5.3 Exmaple to Update Job Status Here's an k8s yaml...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~596-~596: The double modal “requires related” is nonstandard (only accepted in certain dialects). Consider “to be related”.
Context: ... completed!" ``` Note: It requires related RBAC permissions configuration (as defi...
(NEEDS_FIXED)
[grammar] ~660-~660: Use a hyphen to join words.
Context: ...r image build and testing - RBAC and k8s related YAML files for GCP validation jo...
(QB_NEW_EN_HYPHEN)
[grammar] ~735-~735: Use a hyphen to join words.
Context: ...n status - [ ] Basic timeout handling (5 minute max wait), if validation report f...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
3-3: Bare URL used
(MD034, no-bare-urls)
6-6: Bare URL used
(MD034, no-bare-urls)
6-6: Bare URL used
(MD034, no-bare-urls)
37-37: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
40-40: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
48-48: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
52-52: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
173-173: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
523-523: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
539-539: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
651-651: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
652-652: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
669-669: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
670-670: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
671-671: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
727-727: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (1)
1-10: Spike report is comprehensive and well-structured.The document effectively consolidates exploration, technical specifications, and implementation roadmap. It successfully addresses feedback from previous reviews and provides clear acceptance criteria for MVP implementation. The two-container sidecar pattern, environment-driven configuration, and phased roadmap are well-justified.
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
Outdated
Show resolved
Hide resolved
b7ccd6c to
36bf1b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (2)
596-596: Improve awkward phrasing.The phrase uses nonstandard double modal construction. Rephrase for clarity and directness.
-**Note:** It requires related RBAC permissions configuration (as defined in Section 4.3) to update the job status. +**Note:** RBAC permissions configuration (as defined in Section 4.3) is required to update the job status.
537-537: Fix typo: "Exmaple" → "Example".This typo was flagged in previous reviews and should be corrected.
-### 5.3 Exmaple to Update Job Status +### 5.3 Example to Update Job Status
🧹 Nitpick comments (4)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (4)
2-6: Replace bare URLs with proper markdown link formatting.Several URLs should be wrapped in markdown link syntax for better readability and proper rendering.
-**JIRA Story**: [HYPERFLEET-59](https://issues.redhat.com/browse/HYPERFLEET-59) -**Prepared By**: dawang@redhat.com -**Date**: November 21, 2025 +**JIRA Story**: [HYPERFLEET-59](https://issues.redhat.com/browse/HYPERFLEET-59) +**Prepared By**: [dawang@redhat.com](mailto:dawang@redhat.com) +**Date**: November 21, 2025 -**Reviewers**: amarin@redhat.com, croche@redhat.com +**Reviewers**: [amarin@redhat.com](mailto:amarin@redhat.com), [croche@redhat.com](mailto:croche@redhat.com)
98-109: Add language specifiers to fenced code blocks for syntax highlighting.Multiple code blocks lack language identifiers. Add language specifiers for better rendering and readability:
- Lines 98-109:
plaintextor remove (ASCII diagram)- Lines 133-139:
plaintext(flow description)- Lines 173-183:
plaintext(calculation example)- Lines 200-231:
go(already has language, good)- Lines 270-296:
go(already has language, good)- Lines 523-535:
plaintextortext(ASCII diagram)- Lines 539-594:
bash(shell script)Example for line 98-109:
-``` +```plaintext Kubernetes Job PodAnd for line 539-594:
-``` +```bash - name: status-updaterAlso applies to: 133-139, 173-183, 200-231, 270-296, 523-535, 539-594
651-671: Fix list indentation to match markdown style guide.Several lists have incorrect indentation (4 spaces instead of 2 spaces). This affects lines 651-652, 669-671, and other list items.
GCP validation logic with TWO validators only: - - **Credential validation** - Verify GCP credentials exist and are valid - - **API enablement check** - Check if required APIs are enabled + - **Credential validation** - Verify GCP credentials exist and are valid + - **API enablement check** - Check if required APIs are enabled
37-43: Convert emphasis-only lines to proper markdown headings.Lines that use emphasis (bold/italic) as standalone lines should be converted to proper heading levels for better document structure and navigation.
Examples:
- Line 37:
**Pros**→#### Pros- Line 40:
**Cons**→#### Cons- Line 48:
**Pros**→#### Pros- Line 52:
**Cons**→#### Cons- Line 727:
**Ticket: GCP Validation MVP...→ Consider as a proper headingAlso applies to: 48-55, 727-776
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
[style] ~4-~4: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...ang@redhat.com Date: November 21, 2025 Status: Reviewing Reviewers...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~300-~300: Use a hyphen to join words.
Context: ... } } ``` --- ## 4. Validation Adapter Related Configurations ### 4.1 Kubernet...
(QB_NEW_EN_HYPHEN)
[grammar] ~537-~537: Ensure spelling is correct
Context: ... └──────────────────────┘ ``` ### 5.3 Exmaple to Update Job Status Here's an k8s yaml...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~596-~596: The double modal “requires related” is nonstandard (only accepted in certain dialects). Consider “to be related”.
Context: ... completed!" ``` Note: It requires related RBAC permissions configuration (as defi...
(NEEDS_FIXED)
[grammar] ~660-~660: Use a hyphen to join words.
Context: ...r image build and testing - RBAC and k8s related YAML files for GCP validation jo...
(QB_NEW_EN_HYPHEN)
[grammar] ~735-~735: Use a hyphen to join words.
Context: ...n status - [ ] Basic timeout handling (5 minute max wait), if validation report f...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
3-3: Bare URL used
(MD034, no-bare-urls)
6-6: Bare URL used
(MD034, no-bare-urls)
6-6: Bare URL used
(MD034, no-bare-urls)
37-37: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
40-40: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
48-48: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
52-52: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
173-173: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
523-523: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
539-539: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
651-651: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
652-652: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
669-669: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
670-670: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
671-671: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
727-727: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (3)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (3)
313-323: Generation field properly included in Job naming and annotations.The Job name template
gcp-validate-{{.cluster_id}}-{{.generation}}and the annotationgeneration: "{{.generation}}"address the past review feedback about tracking which generation a validation job was created for. This allows the adapter to differentiate between versions and handle scenarios where cluster generation increments.
12-12: Verify Git/GitLab repository URLs are current and accessible.Several internal reference URLs point to GitLab repositories:
- Line 12:
uhc-clusters-serviceGCP preflight logic- Line 44:
update-job-statusrepository- Line 62:
uhc-clusters-serviceimplementation referenceEnsure these references are still valid and haven't been migrated to GitHub or other locations. During implementation, the team should verify link accessibility.
Also applies to: 44-44, 62-62
355-357: TBD items for WIF configuration need clarity before implementation.Two critical TBD items remain:
- Line 355-357:
GCP_DEPLOYER_SA_EMAILenvironment variable configuration- Line 494-496: Complete WIF configuration strategy for platform service account authorization
Recommendation: Schedule a design discussion with the GCP/security team to finalize:
- Whether the platform uses a shared Deployer SA or cluster-specific SAs for WIF impersonation
- How the adapter passes WIF configuration to the validator container
- Cross-project authorization boundaries and impersonation strategy
These decisions should be resolved before the MVP implementation begins, as they affect the job manifest template and validator authentication logic.
Also applies to: 494-496
|
@ciaranRoche Thanks for the great suggestion! I’ve explored the CRD-based approach and added a new section |
36bf1b1 to
69ea221
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (6)
1-50: Approved with minor style improvements.The spike report is well-structured and comprehensive. Past review feedback has been properly incorporated (generation field, ENV variables, backoffLimit, timeout handling, typo fix, phrasing improvements). The design decisions are well-reasoned and the MVP scope is appropriately narrow.
A few style improvements would benefit readability:
- Wrap bare email addresses and URLs (e.g., line 3:
dawang@redhat.com, line 6: reviewer emails) in proper link format or use footnotes.- Several code blocks (lines 98, 133, 173, 523, 539) should specify language tags (e.g.,
```yaml,```go,```bash) for syntax highlighting.
300-300: Improve hyphenation for compound adjectives.Use en-dash or hyphen to join compound adjectives: "Validation Adapter–Related Configurations" or "Validation Adapter-Related Configurations".
447-461: Clarify Workload Identity Federation (WIF) annotation resolution.The service account annotation at line 461 is marked as TBD. Before MVP, clarify:
- Whether the GCP service account email is fixed per environment (shared platform identity) or variable per deployment.
- How adapters discover or receive this annotation value during Job creation.
- Whether this annotation is set by the adapter framework or pre-configured on the cluster.
This is acceptable for spike but must be resolved before implementation begins.
725-776: Acceptance criteria are well-defined; minor clarification needed on WIF setup scope.The MVP acceptance criteria are specific, measurable, and appropriately scoped to two validators. However, clarify the scope of "Workload Identity Federation for GCP authentication" acceptance criterion (lines 740–742):
- Does the MVP require full WIF setup instructions (manual + automated), or is a proof-of-concept setup guide sufficient?
- Should acceptance criteria include a test case verifying WIF authentication works end-to-end, or is documentation of the setup process sufficient?
- Are troubleshooting instructions required for failed WIF configurations in MVP?
Suggested refinement: Add a specific AC like: "[ ] Provide setup documentation with step-by-step WIF configuration for test GCP project (automated scripts optional, post-MVP)".
651-671: Fix list indentation for consistency.Lines 651–671 show inconsistent list indentation (4 spaces instead of standard 2 spaces). Apply consistent indentation throughout the implementation plan section:
- **Scope**: Absolute minimum to validate the solution architecture: - - Status reporter sidecar (basic version) - - GCP validation logic with TWO validators only: - - **Credential validation** - Verify GCP credentials exist and are valid - - **API enablement check** - Check if required APIs are enabled +- **Scope**: Absolute minimum to validate the solution architecture: + - Status reporter sidecar (basic version) + - GCP validation logic with TWO validators only: + - **Credential validation** - Verify GCP credentials exist and are valid + - **API enablement check** - Check if required APIs are enabled
739-755: Clarify scope of WIF validation in Validator 1 vs Validator 2.The acceptance criteria distinguish between two validators:
- Validator 1 (WIF Check): Only verifies annotation exists; does not test WIF functionality (line 743).
- Validator 2 (API Enablement): Uses WIF to actually test authentication + API access (lines 747–754).
This is appropriately scoped for MVP. However, ensure the JSON report format distinguishes between these two check types so the adapter framework can differentiate "annotation exists but WIF untested" from "WIF works and APIs enabled". Propose adding to acceptance criteria:
- Validation report JSON schema clearly distinguishes between WIF configuration check (annotation) and WIF functionality check (API access).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
[style] ~4-~4: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...ang@redhat.com Date: November 21, 2025 Status: Reviewing Reviewers...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~300-~300: Use a hyphen to join words.
Context: ... } } ``` --- ## 4. Validation Adapter Related Configurations ### 4.1 Kubernet...
(QB_NEW_EN_HYPHEN)
[grammar] ~660-~660: Use a hyphen to join words.
Context: ...r image build and testing - RBAC and k8s related YAML files for GCP validation jo...
(QB_NEW_EN_HYPHEN)
[grammar] ~735-~735: Use a hyphen to join words.
Context: ...n status - [ ] Basic timeout handling (5 minute max wait), if validation report f...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md
3-3: Bare URL used
(MD034, no-bare-urls)
6-6: Bare URL used
(MD034, no-bare-urls)
6-6: Bare URL used
(MD034, no-bare-urls)
37-37: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
40-40: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
48-48: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
52-52: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
173-173: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
523-523: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
539-539: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
651-651: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
652-652: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
669-669: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
670-670: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
671-671: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
727-727: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (4)
hyperfleet/components/adapter/validation/GCP/gcp-validation-adapter-spike-report.md (4)
427-439: Configuration strategy is well-designed but verify ENV variable naming conventions.The decision to use Environment Variables for cluster-specific configuration (GCP_PROJECT_ID, GCP_REGION) and VALIDATION_CHECKS is practical and aligns with Kubernetes best practices. Ensure ENV variable naming is consistent across all validator implementations (use SCREAMING_SNAKE_CASE throughout). Also confirm that VALIDATION_CHECKS values are validated or documented (line 363: comma-separated list—specify expected values and validation rules for this field).
313-425: Job manifest template is well-structured; verify template variable syntax consistency.The two-container pattern is appropriate, and the inclusion of the generation field (line 323) and activeDeadlineSeconds (line 423) addresses prior concerns. However, ensure that template variable syntax (e.g.,
{{.cluster_id}},{{.namespace}}) is consistent with the actual template engine used by the adapter framework. Verify this matches the templating patterns used elsewhere in the codebase.
537-596: Status Reporter example is practical; verify fallback error handling is documented.The example implementation at line 539 demonstrates a working pattern using kubectl. The improved phrasing at line 596 clarifies RBAC requirements. However, verify that the spike or MVP acceptance criteria document:
- Error handling scenarios: What happens if the results JSON is malformed or missing fields? The example should gracefully handle parsing errors.
- Timeout behavior: The example polls without explicit timeout. Confirm this relies on Job's
activeDeadlineSeconds(line 423) and is clearly documented.- Fallback to Job status: As noted in prior comments, if validator crashes before writing results, the fallback uses Job status. Confirm this fallback logic is implemented in the reporter sidecar and tested.
600-641: CRD alternative approach section is thorough; decision is clear and justified.Section 6 provides a well-reasoned trade-off analysis between Job Status and CRD-based approaches. The decision to use Job Status for MVP (lines 621–627) is sound and justified. The post-MVP reconsideration criteria (lines 629–635) are also clear. No changes required; this section is well-done.
This PR adds a comprehensive spike report about GCP validation adapter, which includes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.