WIP: experimentation with AI docs#975
Draft
rvanderp3 wants to merge 1 commit intoopenshift:masterfrom
Draft
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rvanderp3 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Author
|
/test ? |
Contributor
|
@rvanderp3: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
|
@rvanderp3: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
needs-rebase
WalkthroughAdds extensive documentation and ADRs for the Cloud Credential Operator: development and testing guides, architecture and domain docs, execution-plan and ecosystem references, four ADRs and an ADR template, plus an updated AGENTS.md. Changes span 11 files and ~2,130 lines of documentation edits/additions. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 7 minutes and 6 seconds. Comment |
rvanderp3
force-pushed
the
vsphere-multi-creds
branch
from
d1f4173 to
04e601a
Compare
openshift-ci
Bot
removed
the
needs-rebase
rvanderp3
force-pushed
the
vsphere-multi-creds
branch
from
04e601a to
3c1749e
Compare
rvanderp3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
ai-docs/CCO_TESTING.md (1)
59-89: ⚡ Quick winMake the “fake client” test snippet less copy/paste-dangerous.
The controller unit test example uses
fake.NewFakeClient()and omits the imports/setup needed to compile. If this isn’t an actual helper in the codebase, readers may copy it and fail immediately. If it is real, it’s still safer to ensure the snippet matches the project’s test patterns (or explicitly label it as pseudo-code).Consider either:
- using the actual fake client construction pattern used elsewhere in this repo, or
- adding a clear
// PSEUDOCODE/// Replace with your test fake client setupnote.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai-docs/CCO_TESTING.md` around lines 59 - 89, Update the TestCredentialsRequestReconcile snippet to avoid misleading readers by either replacing fake.NewFakeClient() with the repository's real test client factory or by adding a clear pseudocode note; specifically, annotate the fake client creation (fake.NewFakeClient) with a comment like "// PSEUDOCODE: replace with your project's test client setup" or swap it to the actual helper used in tests, and ensure the snippet mentions any required setup for Reconciler, mockActuator, ctx and req so readers copying TestCredentialsRequestReconcile and calling r.Reconcile(ctx, req) won't get compile errors.ai-docs/architecture/components.md (1)
83-134: ⚡ Quick winClarify metrics vs mode taxonomy (manual vs manual-pod-identity).
Line 87-88 defines mode annotation values as
mint | passthrough | manual | manual-pod-identity, but Line 132-134 mapscco_credentials_modegauge as0=unknown, 1=mint, 2=passthrough, 3=manual. If3=manualalso coversmanual-pod-identity, this should be explicitly stated; otherwise the docs currently look inconsistent with the “four modes” model (Line 137).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai-docs/architecture/components.md` around lines 83 - 134, The docs are inconsistent about the mode taxonomy vs metric values: clarify how the annotation cloudcredential.openshift.io/mode values (mint | passthrough | manual | manual-pod-identity) map to the cco_credentials_mode gauge; either state explicitly that cco_credentials_mode=3 covers both manual and manual-pod-identity, or add a separate value (e.g., 4=manual-pod-identity) and update the metrics list accordingly; reference the annotation name and the metric name cco_credentials_mode and adjust the text under the "secretannotator Controller" and "metrics Controller" sections to reflect the chosen mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ai-docs/architecture/components.md`:
- Around line 30-54: The architecture overview diagram's fenced code block
triggers markdownlint MD040 because it lacks a language; update the diagram's
opening fence to include the "text" language identifier so the block becomes a
text code fence (fix the fenced block in the Architecture Overview section of
ai-docs/architecture/components.md where the diagram is defined), i.e., replace
the bare triple backticks with triple backticks followed by text to silence
MD040.
- Around line 280-290: The fenced code block in the "Mint Mode Flow" section
triggers markdownlint MD040 because it lacks a language; update the opening
fence for the flow diagram from ``` to ```text so the block becomes a text code
fence and resolves MD040; locate the block under the "Mint Mode Flow" heading in
ai-docs/architecture/components.md (the sequence listing steps 1–7) and change
only the opening fence token to include the language identifier.
- Around line 335-355: The fenced diagram under the "Component Relationships"
heading uses a bare code fence; add the language identifier "text" to the
opening fence (i.e., change ``` to ```text) so markdownlint MD040 is satisfied;
update the fenced block that contains the CredentialsRequest / Root Credential
Secret / ClusterOperator / Pod Identity Webhook diagram (the code block shown in
the diff) to begin with ```text and leave the closing fence unchanged.
- Around line 292-305: The fenced diagram under the "Manual + OIDC Flow" heading
uses a bare ``` fence and triggers markdownlint MD040; update the opening fence
to include a language tag by changing the opening backticks from ``` to ```text
for the code block that lists steps 1–8 (the fenced block beneath the "Manual +
OIDC Flow" header) so the block is recognized as text and MD040 is satisfied.
- Around line 5-28: The fenced code block in the Repository Structure section
triggers MD040 because it lacks a language; edit the opening fence of the
triple-backtick block (the repository structure code block) to use ```text
instead of ``` so the block becomes a text-highlighted code fence and resolves
the lint error.
In `@ai-docs/domain/credentialsrequest.md`:
- Around line 133-140: Update the "Mode Detection" bullets in the Mode Detection
section to match ADR-0001’s algorithm: make the first check be the presence of
the root credential secret (if missing → Manual mode), then the cloud API mint
capability check (can mint?) and finally the credential satisfaction/ODIC vs
passthrough logic; replace the ambiguous "all secrets exist" wording with an
explicit "root credential secret exists" gate and mention the subsequent OIDC vs
Manual+OIDC selection decision as described in ADR-0001 so the detection order
and gating match the ADR precisely.
- Around line 149-165: The docs list for "Common conditions" is out of sync with
the CRD condition-type constants; update the list in the
CredentialsRequestCondition section so the condition names exactly match the
enum/constant names used in the code (e.g. use CredentialsProvisionFailure,
InsufficientCloudCreds, MissingTargetNamespace, CredentialsDeprovisionFailure,
Ignored, StaleCredentials, OrphanedCloudResource instead of
CloudCredSecretNotFound and ProvisionedCredentialsFailed), and verify each
listed name matches the corresponding constant string used by the
CredentialsRequestCondition type.
---
Nitpick comments:
In `@ai-docs/architecture/components.md`:
- Around line 83-134: The docs are inconsistent about the mode taxonomy vs
metric values: clarify how the annotation cloudcredential.openshift.io/mode
values (mint | passthrough | manual | manual-pod-identity) map to the
cco_credentials_mode gauge; either state explicitly that cco_credentials_mode=3
covers both manual and manual-pod-identity, or add a separate value (e.g.,
4=manual-pod-identity) and update the metrics list accordingly; reference the
annotation name and the metric name cco_credentials_mode and adjust the text
under the "secretannotator Controller" and "metrics Controller" sections to
reflect the chosen mapping.
In `@ai-docs/CCO_TESTING.md`:
- Around line 59-89: Update the TestCredentialsRequestReconcile snippet to avoid
misleading readers by either replacing fake.NewFakeClient() with the
repository's real test client factory or by adding a clear pseudocode note;
specifically, annotate the fake client creation (fake.NewFakeClient) with a
comment like "// PSEUDOCODE: replace with your project's test client setup" or
swap it to the actual helper used in tests, and ensure the snippet mentions any
required setup for Reconciler, mockActuator, ctx and req so readers copying
TestCredentialsRequestReconcile and calling r.Reconcile(ctx, req) won't get
compile errors.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 859e4f83-7269-4b74-a8ff-465fa932c5c2
📒 Files selected for processing (10)
ai-docs/CCO_DEVELOPMENT.mdai-docs/CCO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-automatic-mode-detection.mdai-docs/decisions/adr-0002-ccoctl-off-cluster-tool.mdai-docs/decisions/adr-0003-finalizer-for-cloud-cleanup.mdai-docs/decisions/adr-template.mdai-docs/domain/credentialsrequest.mdai-docs/exec-plans/README.mdai-docs/references/ecosystem.md
Comment on lines
+5
to
+28
| ## Repository Structure | ||
|
|
||
| ``` | ||
| cloud-credential-operator/ | ||
| ├── cmd/ | ||
| │ ├── cloud-credential-operator/ # Main operator binary | ||
| │ └── ccoctl/ # Off-cluster CLI tool | ||
| ├── pkg/ | ||
| │ ├── apis/cloudcredential/v1/ # CredentialsRequest CRD | ||
| │ ├── operator/ # Controllers | ||
| │ ├── aws/ # AWS actuator | ||
| │ ├── azure/ # Azure actuator | ||
| │ ├── gcp/ # GCP actuator | ||
| │ ├── ibmcloud/ # IBM Cloud actuator | ||
| │ ├── kubevirt/ # KubeVirt actuator | ||
| │ ├── nutanix/ # Nutanix actuator | ||
| │ ├── openstack/ # OpenStack actuator | ||
| │ ├── ovirt/ # oVirt actuator | ||
| │ ├── powervs/ # PowerVS actuator | ||
| │ ├── vsphere/ # vSphere actuator | ||
| │ └── cmd/ # CLI commands (ccoctl) | ||
| ├── manifests/ # Operator deployment manifests | ||
| └── test/ # Test suites | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint MD040: add a language to the repository structure code fence.
The fenced code block starting at Line 7 uses bare ``` and will trip MD040 (“Fenced code blocks should have a language specified”). Add text to the opening fence.
🛠️ Proposed change
-```
+```text
cloud-credential-operator/
├── cmd/
│ ├── cloud-credential-operator/ # Main operator binary
│ └── ccoctl/ # Off-cluster CLI tool
├── pkg/
│ ├── apis/cloudcredential/v1/ # CredentialsRequest CRD
│ ├── operator/ # Controllers
│ ├── aws/ # AWS actuator
│ ├── azure/ # Azure actuator
│ ├── gcp/ # GCP actuator
│ ├── ibmcloud/ # IBM Cloud actuator
│ ├── kubevirt/ # KubeVirt actuator
│ ├── nutanix/ # Nutanix actuator
│ ├── openstack/ # OpenStack actuator
│ ├── ovirt/ # oVirt actuator
│ ├── powervs/ # PowerVS actuator
│ ├── vsphere/ # vSphere actuator
│ └── cmd/ # CLI commands (ccoctl)
├── manifests/ # Operator deployment manifests
└── test/ # Test suites
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` around lines 5 - 28, The fenced code
block in the Repository Structure section triggers MD040 because it lacks a
language; edit the opening fence of the triple-backtick block (the repository
structure code block) to use ```text instead of ``` so the block becomes a
text-highlighted code fence and resolves the lint error.
Comment on lines
+30
to
+54
| ## Architecture Overview | ||
|
|
||
| ``` | ||
| ┌─────────────────────────────────────────────────────────┐ | ||
| │ Cloud Credential Operator │ | ||
| │ │ | ||
| │ ┌────────────────┐ ┌────────────────┐ ┌───────────┐ │ | ||
| │ │ credentialsreq │ │ secretannotator│ │ status │ │ | ||
| │ │ controller │ │ controller │ │ controller│ │ | ||
| │ └───────┬────────┘ └────────────────┘ └───────────┘ │ | ||
| │ │ │ | ||
| │ ├─> Mode Detection (Mint/Passthrough/Manual) │ | ||
| │ │ │ | ||
| │ ├─> Cloud Provider Actuator │ | ||
| │ │ ┌─────────────────────────────────────┐ │ | ||
| │ └─> │ AWS │ Azure │ GCP │ IBM │ ... │ │ │ | ||
| │ └─────────────────────────────────────┘ │ | ||
| └─────────────────────────────────────────────────────────┘ | ||
| │ | ||
| v | ||
| ┌────────────────────────┐ | ||
| │ Cloud Provider API │ | ||
| │ (IAM, Service Acct) │ | ||
| └────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint MD040: add a language to the architecture overview diagram fence.
The fenced code block starting at Line 32 uses bare ``` and triggers MD040. Use text for the opening fence.
🛠️ Proposed change
-```
+```text
┌─────────────────────────────────────────────────────────┐
│ Cloud Credential Operator │
│ │
│ ┌────────────────┐ ┌────────────────┐ ┌───────────┐ │
│ │ credentialsreq │ │ secretannotator│ │ status │ │
│ │ controller │ │ controller │ │ controller│ │
│ └───────┬────────┘ └────────────────┘ └───────────┘ │
│ │ │
│ ├─> Mode Detection (Mint/Passthrough/Manual) │
│ │ │
│ ├─> Cloud Provider Actuator │
│ │ ┌─────────────────────────────────────┐ │
│ └─> │ AWS │ Azure │ GCP │ IBM │ ... │ │ │
│ └─────────────────────────────────────┘ │
└─────────────────────────────────────────────────────────┘
│
v
┌────────────────────────┐
│ Cloud Provider API │
│ (IAM, Service Acct) │
└────────────────────────┘
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` around lines 30 - 54, The architecture
overview diagram's fenced code block triggers markdownlint MD040 because it
lacks a language; update the diagram's opening fence to include the "text"
language identifier so the block becomes a text code fence (fix the fenced block
in the Architecture Overview section of ai-docs/architecture/components.md where
the diagram is defined), i.e., replace the bare triple backticks with triple
backticks followed by text to silence MD040.
Comment on lines
+280
to
+290
| ### Mint Mode Flow | ||
|
|
||
| ``` | ||
| 1. Component creates CredentialsRequest | ||
| 2. credentialsrequest controller detects CR | ||
| 3. Controller calls cloud actuator.Create() | ||
| 4. Actuator creates cloud IAM resource (user/role) | ||
| 5. Actuator creates Secret with credentials | ||
| 6. Controller updates CR status (Provisioned: true) | ||
| 7. Component uses credentials from Secret | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint MD040: add a language to the Mint Mode flow diagram fence.
The fenced code block starting at Line 282 uses bare ``` and triggers MD040. Use text for the opening fence.
🛠️ Proposed change
-```
+```text
1. Component creates CredentialsRequest
2. credentialsrequest controller detects CR
3. Controller calls cloud actuator.Create()
4. Actuator creates cloud IAM resource (user/role)
5. Actuator creates Secret with credentials
6. Controller updates CR status (Provisioned: true)
7. Component uses credentials from Secret
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Mint Mode Flow | |
| ``` | |
| 1. Component creates CredentialsRequest | |
| 2. credentialsrequest controller detects CR | |
| 3. Controller calls cloud actuator.Create() | |
| 4. Actuator creates cloud IAM resource (user/role) | |
| 5. Actuator creates Secret with credentials | |
| 6. Controller updates CR status (Provisioned: true) | |
| 7. Component uses credentials from Secret | |
| ``` | |
| ### Mint Mode Flow | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 282-282: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` around lines 280 - 290, The fenced code
block in the "Mint Mode Flow" section triggers markdownlint MD040 because it
lacks a language; update the opening fence for the flow diagram from ``` to
```text so the block becomes a text code fence and resolves MD040; locate the
block under the "Mint Mode Flow" heading in ai-docs/architecture/components.md
(the sequence listing steps 1–7) and change only the opening fence token to
include the language identifier.
Comment on lines
+292
to
+305
| ### Manual + OIDC Flow | ||
|
|
||
| ``` | ||
| 1. Admin runs `ccoctl <cloud> create-all` (off-cluster) | ||
| - Creates cloud OIDC provider + IAM roles | ||
| - Generates secret manifests | ||
| 2. Admin applies secret manifests to cluster | ||
| 3. Component creates CredentialsRequest (CR) | ||
| 4. credentialsrequest controller detects CR | ||
| 5. Controller creates/updates Secret with cloud config (role ARN, token path) | ||
| 6. podidentity webhook injects env vars into component pod | ||
| 7. Component pod uses ServiceAccount token + cloud config | ||
| 8. Cloud SDK exchanges token for temporary credentials | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint MD040: add a language to the Manual + OIDC flow diagram fence.
The fenced code block starting at Line 294 uses bare ``` and triggers MD040. Use text for the opening fence.
🛠️ Proposed change
-```
+```text
1. Admin runs `ccoctl <cloud> create-all` (off-cluster)
- Creates cloud OIDC provider + IAM roles
- Generates secret manifests
2. Admin applies secret manifests to cluster
3. Component creates CredentialsRequest (CR)
4. credentialsrequest controller detects CR
5. Controller creates/updates Secret with cloud config (role ARN, token path)
6. podidentity webhook injects env vars into component pod
7. Component pod uses ServiceAccount token + cloud config
8. Cloud SDK exchanges token for temporary credentials
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` around lines 292 - 305, The fenced
diagram under the "Manual + OIDC Flow" heading uses a bare ``` fence and
triggers markdownlint MD040; update the opening fence to include a language tag
by changing the opening backticks from ``` to ```text for the code block that
lists steps 1–8 (the fenced block beneath the "Manual + OIDC Flow" header) so
the block is recognized as text and MD040 is satisfied.
Comment on lines
+335
to
+355
| ## Component Relationships | ||
|
|
||
| ``` | ||
| CredentialsRequest (CR) | ||
| ├─ Referenced by Component (e.g., image-registry, ingress) | ||
| ├─ Reconciled by credentialsrequest controller | ||
| ├─ Target Secret created/updated | ||
| └─ Status aggregated by status controller | ||
|
|
||
| Root Credential Secret (kube-system) | ||
| ├─ Used by cloud actuators (mint/passthrough) | ||
| ├─ Annotated by secretannotator controller | ||
| └─ May be removed after install (mint mode only) | ||
|
|
||
| ClusterOperator (cloud-credential) | ||
| └─ Status set by status controller | ||
|
|
||
| Pod Identity Webhook | ||
| ├─ Deployed by podidentity controller | ||
| └─ Mutates pods using OIDC ServiceAccounts | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint MD040: add a language to the Component Relationships diagram fence.
The fenced code block starting at Line 337 uses bare ``` and triggers MD040. Use text for the opening fence.
🛠️ Proposed change
-```
+```text
CredentialsRequest (CR)
├─ Referenced by Component (e.g., image-registry, ingress)
├─ Reconciled by credentialsrequest controller
├─ Target Secret created/updated
└─ Status aggregated by status controller
Root Credential Secret (kube-system)
├─ Used by cloud actuators (mint/passthrough)
├─ Annotated by secretannotator controller
└─ May be removed after install (mint mode only)
ClusterOperator (cloud-credential)
└─ Status set by status controller
Pod Identity Webhook
├─ Deployed by podidentity controller
└─ Mutates pods using OIDC ServiceAccounts
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 337-337: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` around lines 335 - 355, The fenced
diagram under the "Component Relationships" heading uses a bare code fence; add
the language identifier "text" to the opening fence (i.e., change ``` to
```text) so markdownlint MD040 is satisfied; update the fenced block that
contains the CredentialsRequest / Root Credential Secret / ClusterOperator / Pod
Identity Webhook diagram (the code block shown in the diff) to begin with
```text and leave the closing fence unchanged.
Comment on lines
+133
to
+140
| ### Mode Detection | ||
|
|
||
| CCO auto-detects mode on startup by checking root credential capabilities: | ||
| 1. Attempts to query cloud API (can mint?) | ||
| 2. Checks if root credential satisfies all CredentialsRequests (passthrough?) | ||
| 3. Checks if all secrets exist (manual?) | ||
|
|
||
| **Mode is cluster-wide** - all CredentialsRequests use the same mode. |
There was a problem hiding this comment.
Align the simplified “Mode Detection” bullets with ADR-0001’s documented algorithm.
The “Mode Detection” section currently summarizes as:
- query cloud API (can mint?)
- check root credential satisfies all CredentialsRequests (passthrough?)
- check if all secrets exist (manual?)
But ADR-0001’s detection order starts with root credential secret existence (missing → Manual). “All secrets exist” is ambiguous and doesn’t match the more precise first-step gating described in ADR-0001.
Suggest revising step 3 (and possibly the step ordering) to explicitly reference root credential secret presence and the subsequent OIDC/manual+OIDC selection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/domain/credentialsrequest.md` around lines 133 - 140, Update the
"Mode Detection" bullets in the Mode Detection section to match ADR-0001’s
algorithm: make the first check be the presence of the root credential secret
(if missing → Manual mode), then the cloud API mint capability check (can mint?)
and finally the credential satisfaction/ODIC vs passthrough logic; replace the
ambiguous "all secrets exist" wording with an explicit "root credential secret
exists" gate and mention the subsequent OIDC vs Manual+OIDC selection decision
as described in ADR-0001 so the detection order and gating match the ADR
precisely.
Comment on lines
+149
to
+165
| ### Status Conditions | ||
|
|
||
| ```go | ||
| type CredentialsRequestCondition struct { | ||
| Type CredentialsRequestConditionType // CredentialsProvisionFailure, InsufficientCloudCreds, etc. | ||
| Status ConditionStatus // True, False, Unknown | ||
| Reason string // MachineReadable reason | ||
| Message string // Human-readable detail | ||
| } | ||
| ``` | ||
|
|
||
| **Common conditions**: | ||
| - `CredentialsProvisionFailure`: Cloud API error during provisioning | ||
| - `InsufficientCloudCreds`: Root credential lacks permissions | ||
| - `CloudCredSecretNotFound`: Root credential secret missing | ||
| - `ProvisionedCredentialsFailed`: Secret exists but credentials invalid | ||
|
|
There was a problem hiding this comment.
Fix likely out-of-sync “Common conditions” names for CredentialsRequest.
The “Common conditions” list includes CloudCredSecretNotFound and ProvisionedCredentialsFailed. In the provided CRD condition-type constants snippet, the condition types enumerated include CredentialsProvisionFailure, InsufficientCloudCreds, MissingTargetNamespace, CredentialsDeprovisionFailure, Ignored, StaleCredentials, and OrphanedCloudResource—but not the two names above.
Update the docs to match the actual condition-type strings, otherwise operators/tests will be inconsistent with the CRD/status they observe.
🛠️ Proposed doc correction
## Common conditions:
- `CredentialsProvisionFailure`: Cloud API error during provisioning
- `InsufficientCloudCreds`: Root credential lacks permissions
-- `CloudCredSecretNotFound`: Root credential secret missing
-- `ProvisionedCredentialsFailed`: Secret exists but credentials invalid
+- `MissingTargetNamespace`: Target namespace/secret namespace missing
+- `CredentialsDeprovisionFailure`: Error while deleting cloud resources
+- `Ignored`: ProviderSpec doesn’t match cluster infrastructure platform
+- `StaleCredentials`: CredentialsRequest no longer required and needs cleanup
+- `OrphanedCloudResource`: Pivot cleanup failure (e.g., AWS service principal / app registration)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/domain/credentialsrequest.md` around lines 149 - 165, The docs list
for "Common conditions" is out of sync with the CRD condition-type constants;
update the list in the CredentialsRequestCondition section so the condition
names exactly match the enum/constant names used in the code (e.g. use
CredentialsProvisionFailure, InsufficientCloudCreds, MissingTargetNamespace,
CredentialsDeprovisionFailure, Ignored, StaleCredentials, OrphanedCloudResource
instead of CloudCredSecretNotFound and ProvisionedCredentialsFailed), and verify
each listed name matches the corresponding constant string used by the
CredentialsRequestCondition type.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 17: The documented agent mode lists omit the fourth mode; update every
abbreviated mode mention (e.g., the line describing "CCO Controller" and other
mode lists at the locations noted) to consistently reference all four modes:
"Mint, Passthrough, Manual, and Manual + OIDC". Edit the phrases that currently
read "Mint/Passthrough/Manual" to "Mint/Passthrough/Manual/Manual + OIDC" (or
similar grammatical form), and verify the same change is applied to the other
occurrences referenced (the mentions around lines 41 and 87) so the document
uniformly reflects the four-mode model.
- Around line 7-9: The two consecutive blockquote lines containing "**Agent
Instruction**: When working on CCO..." and "**Generic Platform Patterns**: See
[Tier 1 Hub](...)" are split by a blank line which violates markdownlint MD028;
remove the empty line so the blockquote remains contiguous and ensure both lines
start with a leading '>' (e.g., "> **Agent Instruction**..." and "> **Generic
Platform Patterns**...") to fix the formatting for the blockquote rendering and
satisfy the linter.
- Line 84: The Markdown fenced code block in AGENTS.md is missing a language
specifier which triggers MD040; update the opening fence to include a language
(e.g., change "```" to "```text") for the example block that starts with
"CredentialsRequest (CR)" so the block is recognized as plain text by
markdownlint and consistent with the suggested diff; ensure the closing fence
remains "```".
In `@ai-docs/architecture/components.md`:
- Line 72: Update the stale "Mode Detection" references that point to the old
actuator.go location: find the current mode-detection implementation in the repo
(the file that now contains the credentials request actuator/mode detection
logic) and replace the outdated reference text under the "Mode Detection"
section so it points to the correct/current source file; ensure both occurrences
(the two mentions of the actuator reference in the document) are updated to the
new filename/path and wording so readers are directed to the live code.
- Around line 201-208: The Actuator interface snippet is missing required
methods and should be replaced with the real interface: add the methods
GetCredentialsRootSecretLocation, IsTimedTokenCluster, Upgradeable, and
GetCredentialsRootSecret to the Actuator definition so it matches the actual
implementation; ensure the method names and signatures exactly match the real
codebase (same parameters and return types as the implementations of
Actuator.Create, Update, Delete, Exists) and update the snippet text so
implementers are not misled.
In `@ai-docs/decisions/adr-0001-automatic-mode-detection.md`:
- Around line 22-23: Update the ADR wording to reflect that mode detection
occurs during reconciliation rather than only at startup: change the phrase
"automatically detect the appropriate mode at startup" to indicate detection
happens during controller reconciliation (e.g., when
ReconcileCredentialsRequest.Reconcile runs) and similarly update the other
occurrences mentioned (lines 64-67) so the document accurately states detection
happens at reconcile-time by probing cloud API capabilities and cluster state.
- Line 110: Update the incorrect reference link by replacing the current target
with two correct references: the Actuator implementation (look for the Actuator
type and its constructor in actuator/actuator.go) and the reconcile-mode logic
implemented in the Reconcile (or reconcile) method in
credentialsrequest_controller.go; ensure the markdown at line 110 points to
those two correct files/components so readers can find the Actuator
implementation and the Reconcile logic for mode detection.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c68bcfda-1ac2-49f5-a7e0-67b6fd5321d6
📒 Files selected for processing (11)
AGENTS.mdai-docs/CCO_DEVELOPMENT.mdai-docs/CCO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-automatic-mode-detection.mdai-docs/decisions/adr-0002-ccoctl-off-cluster-tool.mdai-docs/decisions/adr-0003-finalizer-for-cloud-cleanup.mdai-docs/decisions/adr-template.mdai-docs/domain/credentialsrequest.mdai-docs/exec-plans/README.mdai-docs/references/ecosystem.md
✅ Files skipped from review due to trivial changes (6)
- ai-docs/decisions/adr-template.md
- ai-docs/references/ecosystem.md
- ai-docs/domain/credentialsrequest.md
- ai-docs/decisions/adr-0002-ccoctl-off-cluster-tool.md
- ai-docs/decisions/adr-0003-finalizer-for-cloud-cleanup.md
- ai-docs/exec-plans/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ai-docs/CCO_DEVELOPMENT.md
- ai-docs/CCO_TESTING.md
Comment on lines
+7
to
+9
| > **Agent Instruction**: When working on CCO, read relevant files from `ai-docs/` for component-specific details. For generic operator patterns, testing practices, or security guidelines, retrieve from [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs). | ||
|
|
||
| ## Common development commands | ||
| The project uses `make` for automation. | ||
| > **Generic Platform Patterns**: See [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs) |
There was a problem hiding this comment.
Fix blockquote formatting to satisfy markdownlint MD028.
There is a blank line splitting consecutive blockquote lines, which triggers no-blanks-blockquote.
Suggested diff
> **Agent Instruction**: When working on CCO, read relevant files from `ai-docs/` for component-specific details. For generic operator patterns, testing practices, or security guidelines, retrieve from [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs).
-
> **Generic Platform Patterns**: See [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| > **Agent Instruction**: When working on CCO, read relevant files from `ai-docs/` for component-specific details. For generic operator patterns, testing practices, or security guidelines, retrieve from [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs). | |
| ## Common development commands | |
| The project uses `make` for automation. | |
| > **Generic Platform Patterns**: See [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs) | |
| > **Agent Instruction**: When working on CCO, read relevant files from `ai-docs/` for component-specific details. For generic operator patterns, testing practices, or security guidelines, retrieve from [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs). | |
| > **Generic Platform Patterns**: See [Tier 1 Hub](https://github.com/openshift/enhancements/tree/master/ai-docs) |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 8-8: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 7 - 9, The two consecutive blockquote lines
containing "**Agent Instruction**: When working on CCO..." and "**Generic
Platform Patterns**: See [Tier 1 Hub](...)" are split by a blank line which
violates markdownlint MD028; remove the empty line so the blockquote remains
contiguous and ensure both lines start with a leading '>' (e.g., "> **Agent
Instruction**..." and "> **Generic Platform Patterns**...") to fix the
formatting for the blockquote rendering and satisfy the linter.
|
|
||
| ## Core Components | ||
|
|
||
| - **CCO Controller**: Reconciles CredentialsRequests | **ccoctl CLI**: Off-cluster credential management | **Mode Detection**: Mint/Passthrough/Manual selection |
There was a problem hiding this comment.
Keep mode references consistent with the documented four-mode model.
These lines still list only Mint/Passthrough/Manual, but the file later documents Manual + OIDC as a first-class mode. Please align all mode references.
Suggested diff
-- **CCO Controller**: Reconciles CredentialsRequests | **ccoctl CLI**: Off-cluster credential management | **Mode Detection**: Mint/Passthrough/Manual selection
+- **CCO Controller**: Reconciles CredentialsRequests | **ccoctl CLI**: Off-cluster credential management | **Mode Detection**: Mint/Passthrough/Manual/Manual+OIDC selection
...
-| **Modes** | [architecture/components.md](ai-docs/architecture/components.md) | Mint/Passthrough/Manual |
+| **Modes** | [architecture/components.md](ai-docs/architecture/components.md) | Mint/Passthrough/Manual/Manual + OIDC |
...
- │ ├─> Mode detection (Mint/Passthrough/Manual)
+ │ ├─> Mode detection (Mint/Passthrough/Manual/Manual + OIDC)Based on learnings: "Mode detection logic must support four modes: Mint (fine-grained credentials), Passthrough (root credential reuse), Manual (external provisioning), and Manual + OIDC (short-lived tokens)".
Also applies to: 41-41, 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 17, The documented agent mode lists omit the fourth mode;
update every abbreviated mode mention (e.g., the line describing "CCO
Controller" and other mode lists at the locations noted) to consistently
reference all four modes: "Mint, Passthrough, Manual, and Manual + OIDC". Edit
the phrases that currently read "Mint/Passthrough/Manual" to
"Mint/Passthrough/Manual/Manual + OIDC" (or similar grammatical form), and
verify the same change is applied to the other occurrences referenced (the
mentions around lines 41 and 87) so the document uniformly reflects the
four-mode model.
|
|
||
| ## Knowledge Graph | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block (MD040).
The fenced block should specify a language (for example text) to satisfy markdownlint.
Suggested diff
-```
+```text
CredentialsRequest (CR)
├─> credentialsrequest controller
│ ├─> Mode detection (Mint/Passthrough/Manual)
│ ├─> Cloud provider actuator (aws|azure|gcp|...)
│ └─> Secret creation/update
...
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 84, The Markdown fenced code block in AGENTS.md is missing
a language specifier which triggers MD040; update the opening fence to include a
language (e.g., change "```" to "```text") for the example block that starts
with "CredentialsRequest (CR)" so the block is recognized as plain text by
markdownlint and consistent with the suggested diff; ensure the closing fence
remains "```".
| 5. Update CredentialsRequest status | ||
|
|
||
| **Key Logic**: | ||
| - **Mode Detection** (`pkg/operator/credentialsrequest/actuator.go`): |
There was a problem hiding this comment.
Mode-detection code path references are inconsistent with current repo layout.
Line 72 and Line 320 reference pkg/operator/credentialsrequest/actuator.go; that path is stale and will send readers to the wrong place.
🔧 Suggested edit
-- **Mode Detection** (`pkg/operator/credentialsrequest/actuator.go`):
+- **Mode Handling** (`pkg/operator/credentialsrequest/credentialsrequest_controller.go`):
...
-**Detection Logic** (`pkg/operator/credentialsrequest/actuator.go`):
+**Detection Logic** (`pkg/operator/credentialsrequest/credentialsrequest_controller.go`):Also applies to: 320-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` at line 72, Update the stale "Mode
Detection" references that point to the old actuator.go location: find the
current mode-detection implementation in the repo (the file that now contains
the credentials request actuator/mode detection logic) and replace the outdated
reference text under the "Mode Detection" section so it points to the
correct/current source file; ensure both occurrences (the two mentions of the
actuator reference in the document) are updated to the new filename/path and
wording so readers are directed to the live code.
Comment on lines
+201
to
+208
| ```go | ||
| type Actuator interface { | ||
| Create(ctx context.Context, cr *CredentialsRequest) error | ||
| Update(ctx context.Context, cr *CredentialsRequest) error | ||
| Delete(ctx context.Context, cr *CredentialsRequest) error | ||
| Exists(ctx context.Context, cr *CredentialsRequest) (bool, error) | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Actuator interface snippet is incomplete and can mislead implementers.
The snippet omits required methods currently in the real interface (GetCredentialsRootSecretLocation, IsTimedTokenCluster, Upgradeable, GetCredentialsRootSecret).
🧩 Suggested replacement snippet
type Actuator interface {
Create(ctx context.Context, cr *CredentialsRequest) error
Update(ctx context.Context, cr *CredentialsRequest) error
Delete(ctx context.Context, cr *CredentialsRequest) error
Exists(ctx context.Context, cr *CredentialsRequest) (bool, error)
+ GetCredentialsRootSecretLocation() types.NamespacedName
+ IsTimedTokenCluster(client.Client, context.Context, log.FieldLogger) (bool, error)
+ Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition
+ GetCredentialsRootSecret(ctx context.Context, cr *CredentialsRequest) (*corev1.Secret, error)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/architecture/components.md` around lines 201 - 208, The Actuator
interface snippet is missing required methods and should be replaced with the
real interface: add the methods GetCredentialsRootSecretLocation,
IsTimedTokenCluster, Upgradeable, and GetCredentialsRootSecret to the Actuator
definition so it matches the actual implementation; ensure the method names and
signatures exactly match the real codebase (same parameters and return types as
the implementations of Actuator.Create, Update, Delete, Exists) and update the
snippet text so implementers are not misled.
Comment on lines
+22
to
+23
| CCO will **automatically detect** the appropriate mode at startup by probing cloud API capabilities and cluster state, rather than requiring explicit user configuration. | ||
|
|
There was a problem hiding this comment.
Detection timing is described as startup-only, but current behavior is reconcile-time.
Line 22 says detection happens “at startup,” while current controller flow evaluates mode/timed-token state during reconciliation (ReconcileCredentialsRequest.Reconcile). This mismatch can mislead debugging and ops runbooks.
🛠️ Suggested wording update
-CCO will **automatically detect** the appropriate mode at startup by probing cloud API capabilities and cluster state, rather than requiring explicit user configuration.
+CCO will **automatically determine** effective credential behavior during reconciliation by combining operator configuration and cluster/cloud capability checks, rather than requiring explicit per-component configuration.Also applies to: 64-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/decisions/adr-0001-automatic-mode-detection.md` around lines 22 - 23,
Update the ADR wording to reflect that mode detection occurs during
reconciliation rather than only at startup: change the phrase "automatically
detect the appropriate mode at startup" to indicate detection happens during
controller reconciliation (e.g., when ReconcileCredentialsRequest.Reconcile
runs) and similarly update the other occurrences mentioned (lines 64-67) so the
document accurately states detection happens at reconcile-time by probing cloud
API capabilities and cluster state.
| ## References | ||
|
|
||
| - [Operator Modes](../architecture/components.md#modes) | ||
| - [Mode Detection Code](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/actuator.go) |
There was a problem hiding this comment.
Reference path for mode-detection code looks incorrect.
Line 110 points to pkg/operator/credentialsrequest/actuator.go, but the actuator file is under pkg/operator/credentialsrequest/actuator/actuator.go, and reconcile-mode logic lives in pkg/operator/credentialsrequest/credentialsrequest_controller.go.
🔧 Suggested link fix
-- [Mode Detection Code](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/actuator.go)
+- [Reconcile Mode Handling](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/credentialsrequest_controller.go)
+- [Actuator Interface](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/actuator/actuator.go)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [Mode Detection Code](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/actuator.go) | |
| - [Reconcile Mode Handling](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/credentialsrequest_controller.go) | |
| - [Actuator Interface](https://github.com/openshift/cloud-credential-operator/blob/master/pkg/operator/credentialsrequest/actuator/actuator.go) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ai-docs/decisions/adr-0001-automatic-mode-detection.md` at line 110, Update
the incorrect reference link by replacing the current target with two correct
references: the Actuator implementation (look for the Actuator type and its
constructor in actuator/actuator.go) and the reconcile-mode logic implemented in
the Reconcile (or reconcile) method in credentialsrequest_controller.go; ensure
the markdown at line 110 points to those two correct files/components so readers
can find the Actuator implementation and the Reconcile logic for mode detection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit