SANDBOX-1769 | docs: design of the "disabled integrations" feature#1255
Conversation
WalkthroughAdds two proposal documents specifying a denylist Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
MikelAlejoBR
force-pushed
the
SANDBOX-1769-docs-feature-enabled-integrations-ho
branch
2 times, most recently
from
2b57f1a to
58f28b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/proposals/enabled-integrations-design.md (1)
44-47: Document config propagation behavior for incident response.Lines 44-47 describe cache-based loading, but the doc does not define expected propagation timing (or fallback behavior if refresh fails). Since this feature is meant for operational mitigation, add explicit expectations (refresh trigger/interval and stale-config behavior) so operators can rely on it during outages.
As per coding guidelines, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
Also applies to: 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/enabled-integrations-design.md` around lines 44 - 47, Document expected propagation timing and stale-config behavior for the disabledIntegrations flow: state that how often Registration service refreshes ToolchainConfig (the refresh interval or that it reacts to watch events), how commonconfig.LoadLatest is triggered (poll interval or event-driven), what constitutes a successful refresh, and the fallback when refresh fails (e.g., continue serving last-known disabledIntegrations from GET /api/v1/uiconfig, expose a “lastUpdated” timestamp and an “staleAfter” threshold, and define operator actions if data is stale). Reference ToolchainConfig.disabledIntegrations, the Registration service’s use of commonconfig.LoadLatest, the JWT-secured GET /api/v1/uiconfig endpoint, and the UI hide behavior in the doc so operators know timing and recovery expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/enabled-integrations-design.md`:
- Around line 53-57: The doc currently allows "free-form" integration
identifiers which risks drift and invalid values; update the proposal to list
canonical integration IDs (e.g., "openshift-lightspeed", "devspaces", "workato")
and add a contract section for validation: require backend to validate entries
in disabledIntegrations against the canonical set, return a clear error for
unknown IDs, and define expected behavior (ignore unknown with warning vs.
reject request) and UI display rules for validation errors so both backend and
UI stay in sync.
In `@docs/proposals/enabled-integrations-questions.md`:
- Around line 29-47: Document and implement identifier validation semantics for
the disabledIntegrations []string: declare the canonical source of truth for
allowed IDs (an enum in the CRD schema or a registry/lookup like an integrations
registry), specify where validation runs (CRD OpenAPI schema enum, admission
webhook, or registration-service guardrail in ValidateIntegrationIDs), and
define the runtime behavior for unknown values (reject on apply with a clear
error, or accept with a logged warning and no-op). Update the proposal text to
reference the specific symbols/places to change: the disabledIntegrations field,
the CRD schema enum, the admission webhook handler
(validateAdmissionReview/ValidateIntegrationIDs), or the registration-service
registration guard so reviewers know where validation is enforced and what
client-visible errors/logging will occur.
---
Nitpick comments:
In `@docs/proposals/enabled-integrations-design.md`:
- Around line 44-47: Document expected propagation timing and stale-config
behavior for the disabledIntegrations flow: state that how often Registration
service refreshes ToolchainConfig (the refresh interval or that it reacts to
watch events), how commonconfig.LoadLatest is triggered (poll interval or
event-driven), what constitutes a successful refresh, and the fallback when
refresh fails (e.g., continue serving last-known disabledIntegrations from GET
/api/v1/uiconfig, expose a “lastUpdated” timestamp and an “staleAfter”
threshold, and define operator actions if data is stale). Reference
ToolchainConfig.disabledIntegrations, the Registration service’s use of
commonconfig.LoadLatest, the JWT-secured GET /api/v1/uiconfig endpoint, and the
UI hide behavior in the doc so operators know timing and recovery expectations.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f7c077c4-4df4-49eb-8109-87a7f6978e51
📒 Files selected for processing (2)
docs/proposals/enabled-integrations-design.mddocs/proposals/enabled-integrations-questions.md
MikelAlejoBR
changed the title
| 2. Run code generation (`make generate`) to update DeepCopy methods and CRD manifests. | ||
|
|
||
| ### Phase 2: Host operator (`codeready-toolchain/host-operator`) | ||
|
|
||
| 1. Bump the `codeready-toolchain/api` dependency in `go.mod` to pick up the new field, then run `make generate manifests` to regenerate the CRD YAML in `config/crd/bases/`. |
There was a problem hiding this comment.
just a detail - make generate dispatches the generated CRDs as well, there is no manifests target.
probably a room for improving either a command, or a skill, or some other file
These are the design documents used for the "disabled integrations" feature, which aims at showing or hiding the available integrations in Sandbox depending on the configuration specified in the ToolchainConfig resource. SANDBOX-1769
MikelAlejoBR
force-pushed
the
SANDBOX-1769-docs-feature-enabled-integrations-ho
branch
from
58f28b9 to
f7ada0a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/proposals/enabled-integrations-design.md (1)
23-23: Optional: Add language identifier to fenced code block.The ASCII diagram would satisfy the markdown linter by adding a language identifier (e.g.,
textor leave it as-is since the diagram is clear without it).📝 Markdown linter fix (optional)
-``` +```text ┌──────────────┐ ┌──────────────────┐ ┌─────────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/enabled-integrations-design.md` at line 23, The fenced code block containing the ASCII diagram should include a language identifier (e.g., add "text" after the opening ``` of the diagram block) so the markdown linter recognizes it as a plain-text/code fence; update the opening fence in the ASCII diagram block (the fenced code block that begins with ``` and contains the box diagram) to be ```text (or another suitable identifier) and leave the diagram content unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/proposals/enabled-integrations-design.md`:
- Line 23: The fenced code block containing the ASCII diagram should include a
language identifier (e.g., add "text" after the opening ``` of the diagram
block) so the markdown linter recognizes it as a plain-text/code fence; update
the opening fence in the ASCII diagram block (the fenced code block that begins
with ``` and contains the box diagram) to be ```text (or another suitable
identifier) and leave the diagram content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d16d747b-50b8-4c63-b4a1-ad1b03571df6
📒 Files selected for processing (2)
docs/proposals/enabled-integrations-design.mddocs/proposals/enabled-integrations-questions.md
|
|
||
| ## Overview | ||
|
|
||
| Developer Sandbox integrates with several downstream services (e.g. OpenShift Lightspeed, DevSpaces, Workato, etc.). When one of these downstream integrations experiences issues, there is currently no way to hide it from customers without a code change and redeployment. |
There was a problem hiding this comment.
I don't know what Workato is but I feel like we should develop an integration with them
There was a problem hiding this comment.
Oh, actually it's a thing... Whoops
|
|
@MikelAlejoBR: 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. |
| const ( | ||
| IntegrationOpenShift IntegrationName = "openshift" | ||
| IntegrationOpenShiftAI IntegrationName = "openshift-ai" | ||
| IntegrationDevSpaces IntegrationName = "devspaces" | ||
| IntegrationAnsibleAutomationPlatform IntegrationName = "ansible-automation-platform" | ||
| IntegrationOpenShiftVirtualization IntegrationName = "openshift-virtualization" | ||
| ) |
There was a problem hiding this comment.
I'm not really sure if we need all those constants here - basically we could avoid maintaining another list of trial names ( which is already present in the UI code ) , if possible I would just add the field as list of strings and we can configure the names manually IMHO.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, MikelAlejoBR The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
These are the design documents used for the "disabled integrations" feature, which aims at showing or hiding the available integrations in Sandbox depending on the configuration specified in the ToolchainConfig resource.
Related PRs
Jira ticket
[SANDBOX-1769]
Summary by CodeRabbit