Skip to content

feat: add vsphere-component annotation to CredentialsRequest CR (story #39)#4

Open
splatypus-bot wants to merge 2 commits into
masterfrom
story-39-vsphere-component-annotation
Open

feat: add vsphere-component annotation to CredentialsRequest CR (story #39)#4
splatypus-bot wants to merge 2 commits into
masterfrom
story-39-vsphere-component-annotation

Conversation

@splatypus-bot
Copy link
Copy Markdown

@splatypus-bot splatypus-bot commented May 9, 2026

Summary

  • Adds cloudcredential.openshift.io/vsphere-component: machineAPI annotation to the openshift-machine-api-vsphere CredentialsRequest CR
  • Annotation is backward-compatible: CCO ignores it in Passthrough mode (default)
  • Adds manifest annotation unit tests in test/manifest/

Test plan

GOFLAGS=-mod=vendor go test ./test/manifest/... -v

All 3 tests pass:

  • TestMAOVSphereCredentialsRequestAnnotation
  • TestNonVSphereCredentialsRequestsUnmodified
  • TestAnnotationValueIsNotEmpty

References

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests

    • Added test coverage for vSphere credential request annotation validation.
  • Chores

    • Added vSphere component annotation to machine API credentials configuration for improved resource identification.

Add cloudcredential.openshift.io/vsphere-component: machineAPI annotation
to the openshift-machine-api-vsphere CredentialsRequest so CCO can route
per-component credentials in PerComponent mode (story openshift#39).

Add manifest annotation unit tests (test/manifest/).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 9, 2026

Warning

Rate limit exceeded

@rvanderp3 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 57 minutes and 29 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9a64a2cd-0cc1-4d49-99e0-56e29a399bed

📥 Commits

Reviewing files that changed from the base of the PR and between 9dc7837 and 0bf1b1f.

📒 Files selected for processing (1)
  • test/manifest/credentials_request_annotation_test.go
📝 Walkthrough

Walkthrough

This PR adds a metadata annotation to the vSphere CredentialsRequest manifest and validates it with three test cases covering annotation presence, value correctness, and absence on non-vSphere credential requests.

Changes

vSphere CredentialsRequest Annotation

Layer / File(s) Summary
Manifest Annotation
install/0000_30_machine-api-operator_00_credentials-request.yaml
The openshift-machine-api-vsphere CredentialsRequest gains the cloudcredential.openshift.io/vsphere-component: machineAPI annotation.
Test Infrastructure
test/manifest/credentials_request_annotation_test.go
Helper functions parse the credentials-request YAML manifest, unmarshaling documents into structs, and provide lookup by CR name.
Annotation Tests
test/manifest/credentials_request_annotation_test.go
Three test cases verify the vSphere CR has the correct annotation with non-empty value and that non-vSphere CRs (AWS, Azure, GCP, OpenStack) do not include it.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Poem

🐰 A manifest grows a new tag so fine,
CloudCredentials marked for vSphere's design,
Tests hop around to verify all's right,
No empty values, no mistaken sight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a vsphere-component annotation to the CredentialsRequest CR, which is the primary purpose of this pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch story-39-vsphere-component-annotation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/manifest/credentials_request_annotation_test.go`:
- Around line 82-85: The test currently masks missing expected
CredentialsRequests by continuing when findCR(crs, name) returns ok==false;
instead fail fast: replace the continue with a test failure (e.g., call t.Fatalf
or t.Fatalf-like helper) that reports the missing CredentialsRequest name so the
test fails if a non-vSphere CR was deleted/renamed; update the block around cr,
ok := findCR(crs, name) to assert ok and include the variable name in the error
message.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: be7a5955-8767-4b43-9292-4f7a92887b27

📥 Commits

Reviewing files that changed from the base of the PR and between 498bb59 and 9dc7837.

📒 Files selected for processing (2)
  • install/0000_30_machine-api-operator_00_credentials-request.yaml
  • test/manifest/credentials_request_annotation_test.go

Comment thread test/manifest/credentials_request_annotation_test.go
…missing

Replacing 'continue' with t.Fatalf ensures the test fails immediately if
an expected non-vSphere CR (aws, azure, gcp, openstack) is absent from
the manifest. Using 'continue' silently hid regressions where a renamed
or deleted CR would cause the annotation check to be skipped entirely.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@splatypus-bot
Copy link
Copy Markdown
Author

💻 dev — 2026-05-11T14:30:22Z

Fixed the CodeRabbit finding (commit 0bf1b1f):

Fixed: continue silently masked regressions in TestNonVSphereCredentialsRequestsUnmodified

  • Changed continue to t.Fatalf(...) at line 84
  • If an expected non-vSphere CredentialsRequest (aws, azure, gcp, openstack) is renamed or deleted from the manifest, the test now fails immediately rather than silently skipping the annotation check

@splatypus-bot
Copy link
Copy Markdown
Author

💻 dev — 2026-05-11T14:51:33Z

Agentic Review Summary — CodeRabbit Feedback Pass (0bf1b1f)

Change reviewed: continuet.Fatalf in TestNonVSphereCredentialsRequestsUnmodified

Assumptions surfaced:

  1. The test assumes all expected non-vSphere CR names (aws, azure, gcp, openstack) must be present in the manifest — the fix now enforces this assumption explicitly.
  2. t.Fatalf is correct here (stops test immediately) vs t.Errorf (continues) — appropriate since remaining loop iterations depend on the CR being found.
  3. The error message correctly describes the failure mode: renamed or removed CR files.

Adversarial cases verified:

  • Old behavior: if a CR was renamed, the test would silently continue and pass even though the annotation wasn't checked
  • New behavior: test now fails fast with a descriptive message identifying exactly which CR is missing
  • This is a test regression prevention fix — it catches CI cases where the manifest is modified without updating the annotation

Delete Test result: ✓ Deleting this test would allow non-vSphere CRs to carry the vsphere-component annotation without detection

Risk surface gaps: None — this is a pure test quality improvement with no production code impact

Verdict: APPROVE — the continue→Fatalf change correctly closes a false-positive gap in the test suite

@splatypus-bot
Copy link
Copy Markdown
Author

💻 dev — 2026-05-12T19:34:12Z

✅ All PR feedback addressed

Verified that all review feedback has been resolved:

CodeRabbit inline comment (2026-05-09T02:34:34Z):

  • ✅ Fixed in commit 0bf1b1f (2026-05-11T14:30:23Z)
  • Changed continue to t.Fatalf to prevent silent test regressions
  • Test now fails fast when expected non-vSphere CRs are missing

Resolution verified in agentic review (2026-05-11T14:51:33Z):

  • Correctly closes false-positive gap in test suite
  • Enforces assumption that all expected CR names must be present
  • Appropriate use of Fatalf (fail-fast vs continue)

No further action required. Story openshift#39 closed.

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.

1 participant