Skip to content

feat(workspace): add mounts and environment in workspace configuration#15

Merged
feloy merged 1 commit intoopenkaiden:mainfrom
feloy:wsconfig1
Mar 18, 2026
Merged

feat(workspace): add mounts and environment in workspace configuration#15
feloy merged 1 commit intoopenkaiden:mainfrom
feloy:wsconfig1

Conversation

@feloy
Copy link
Copy Markdown
Contributor

@feloy feloy commented Mar 18, 2026

fixes #14

The value/secret fields in the EnvironmentVariable are marked as mutually exclusive, but this is not enforced by the spec (using oneOf). This is due to the generator not providing a convenient implementation for this. The responsability of enforcing this rule will be let to the user

Signed-off-by: Philippe Martin <phmartin@redhat.com>

Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 18, 2026

📝 Walkthrough

Walkthrough

This change introduces schema definitions for workspace configuration, adding strict object validation and three new schemas: WorkspaceConfiguration (updated), Mounts, and EnvironmentVariable. These define structure for mount dependencies, config paths, and environment variable declarations with optional secret references.

Changes

Cohort / File(s) Summary
Workspace Configuration Schema
workspace-configuration/openapi.yaml
Added additionalProperties: false to WorkspaceConfiguration schema with new mounts and environment properties. Introduced new Mounts schema with dependencies and configs array properties. Introduced new EnvironmentVariable schema with required name field and optional value and secret fields.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • kortex-hub/kortex-cli-api#7: Modifies the same WorkspaceConfiguration schema in workspace-configuration/openapi.yaml — the retrieved PR adds initial schema structure while this PR expands it with mounts and environment field definitions.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: adding mounts and environment fields to workspace configuration.
Linked Issues check ✅ Passed The PR implements all requirements from issue #14: mounts with dependencies and configs arrays, plus environment variables with name/value or name/secret support.
Out of Scope Changes check ✅ Passed All changes are directly aligned with issue #14 requirements; no out-of-scope modifications detected beyond the scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The description is related to the changeset, referencing issue #14 and explaining a specific design decision about mutually exclusive fields.

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

📝 Coding Plan
  • Generate coding plan for human review comments

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 the current code and only fix it if needed.

Inline comments:
In `@workspace-configuration/openapi.yaml`:
- Around line 44-58: Update the EnvironmentVariable schema to enforce that
exactly one of "value" or "secret" is provided: keep "name" required and
additionalProperties: false, then replace the loose "value" and "secret"
properties with a oneOf that defines two alternatives (one schema requiring
"value", the other requiring "secret") so a payload must match exactly one
branch (preventing both or neither). Refer to the EnvironmentVariable schema and
the "value" and "secret" properties when making this change.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9dd4523d-bd6a-4ec2-9f76-0119b1c9de4e

📥 Commits

Reviewing files that changed from the base of the PR and between d1dc097 and 1e1f133.

📒 Files selected for processing (1)
  • workspace-configuration/openapi.yaml

Comment on lines +44 to +58
EnvironmentVariable:
type: object
additionalProperties: false
required:
- name
properties:
name:
type: string
description: Name of the environment variable
value:
type: string
description: Value of the environment variable (mutually exclusive with secret)
secret:
type: string
description: Name of the secret containing the value (mutually exclusive with value)
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Mutual exclusivity between value and secret is not enforced.

The descriptions state that value and secret are mutually exclusive, but the current schema:

  1. Allows both to be provided simultaneously
  2. Allows neither to be provided (only name is required)

Per the PR objectives, each environment variable should have either a value or a secret reference.

🛠️ Proposed fix using `oneOf` to enforce mutual exclusivity
     EnvironmentVariable:
       type: object
       additionalProperties: false
       required:
         - name
+      oneOf:
+        - required: [value]
+          not:
+            required: [secret]
+        - required: [secret]
+          not:
+            required: [value]
       properties:
         name:
           type: string
           description: Name of the environment variable
         value:
           type: string
           description: Value of the environment variable (mutually exclusive with secret)
         secret:
           type: string
           description: Name of the secret containing the value (mutually exclusive with value)

Alternatively, a simpler approach if tooling supports it:

     EnvironmentVariable:
       type: object
       additionalProperties: false
       required:
         - name
+      oneOf:
+        - required: [value]
+        - required: [secret]
       properties:
         name:
           type: string
           description: Name of the environment variable
         value:
           type: string
           description: Value of the environment variable (mutually exclusive with secret)
         secret:
           type: string
           description: Name of the secret containing the value (mutually exclusive with value)

Note: The simpler oneOf approach ensures at least one is present but technically allows both (the oneOf would fail if both are provided since both branches would match). If strict exclusivity is needed, use the first approach.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workspace-configuration/openapi.yaml` around lines 44 - 58, Update the
EnvironmentVariable schema to enforce that exactly one of "value" or "secret" is
provided: keep "name" required and additionalProperties: false, then replace the
loose "value" and "secret" properties with a oneOf that defines two alternatives
(one schema requiring "value", the other requiring "secret") so a payload must
match exactly one branch (preventing both or neither). Refer to the
EnvironmentVariable schema and the "value" and "secret" properties when making
this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the oneof implementation for generated code is not very convenient. We will let the users enforce this rule

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@feloy, understood — that's a reasonable trade-off. Codegen tooling often struggles with oneOf compositions, and keeping the schema simple while delegating enforcement to the application layer is a pragmatic choice. I'll keep this in mind for future reviews.


✏️ Learnings added
Learnt from: feloy
Repo: kortex-hub/kortex-cli-api PR: 15
File: workspace-configuration/openapi.yaml:44-58
Timestamp: 2026-03-18T15:44:51.144Z
Learning: In the kortex-cli-api repository, the `EnvironmentVariable` schema in `workspace-configuration/openapi.yaml` intentionally does NOT use `oneOf` to enforce mutual exclusivity between `value` and `secret` fields. The team prefers to keep the schema simple for generated code ergonomics and lets users/application logic enforce the mutual exclusivity rule instead. Do not flag this as an issue in future reviews.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: feloy
Repo: kortex-hub/kortex-cli-api PR: 10
File: cli/openapi.yaml:0-0
Timestamp: 2026-03-11T08:49:39.162Z
Learning: In the kortex-cli-api repository, the OpenAPI spec (cli/openapi.yaml) intentionally documents only CLI command **output** (response schemas and examples). Input parameters (e.g., workspace id passed to `kortex-cli workspace remove <id>`) are deliberately **not** described in the spec. Review comments about missing input/query/path parameters in this spec are not applicable.

@feloy feloy requested review from benoitf and jeffmaury March 18, 2026 15:46
@feloy feloy merged commit 855fd82 into openkaiden:main Mar 18, 2026
2 checks passed
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.

provide workspace config with env vars and mounts

2 participants