Add ephemeral_deploy_rosa MCP tool, split bonfire_mcp into standalone package#590
Open
bsquizz wants to merge 5 commits into
Open
Add ephemeral_deploy_rosa MCP tool, split bonfire_mcp into standalone package#590bsquizz wants to merge 5 commits into
bsquizz wants to merge 5 commits into
Conversation
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughRemoves ChangesROSA Deploy Pipeline
Packaging and Infrastructure
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 100, 200, 0.5)
note over MCPClient,status: ephemeral_deploy_rosa tool invocation
MCPClient->>server_call_tool: ephemeral_deploy_rosa(duration, requester, timeout)
server_call_tool->>_deploy_rosa: validate duration
_deploy_rosa->>reservations: reserve(client, pool="rosa", duration, requester)
reservations-->>_deploy_rosa: namespace
end
rect rgba(100, 180, 100, 0.5)
note over _deploy_rosa,EphemeralK8sClient: deploy_rosa pipeline per component
_deploy_rosa->>get_apps_for_env: get_apps_for_env(target_env, qontract_client)
get_apps_for_env-->>_deploy_rosa: app configs
loop each component
_deploy_rosa->>RepoFile: from_component → fetch() → (commit_sha, content)
_deploy_rosa->>EphemeralK8sClient: process_template(template, params)
EphemeralK8sClient->>OpenShiftAPI: POST processedtemplates
OpenShiftAPI-->>EphemeralK8sClient: expanded objects
_deploy_rosa->>EphemeralK8sClient: apply_resource per object
end
_deploy_rosa->>wait_for_resources: poll CAPI/ClowdApp/Deployment readiness
wait_for_resources-->>_deploy_rosa: all ready
end
rect rgba(200, 100, 100, 0.5)
note over _deploy_rosa,MCPClient: result or cleanup on failure
alt FatalError during deploy
_deploy_rosa->>reservations: release(client, namespace)
_deploy_rosa-->>MCPClient: error CallToolResult
else success
_deploy_rosa->>status: describe_namespace(client, namespace)
_deploy_rosa-->>MCPClient: format_deploy_rosa(result)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Replace the MCP server's subprocess call to `bonfire deploy rosa` with pure Python library calls so `pip install crc-bonfire[mcp]` works without the oc binary, ocviapy, or sh. New modules: - bonfire_lib/qontract.py: app-interface GraphQL client - bonfire_lib/repo_fetch.py: HTTP template fetching from GitHub/GitLab - bonfire_lib/deploy.py: deploy orchestration with readiness polling Extended EphemeralK8sClient with apply_resource(), list_dynamic_resources(), and process_template() (OpenShift Template API). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bsquizz
changed the title
bsquizz
changed the title
The ROSA template creates cluster.x-k8s.io Cluster, ROSACluster, and ROSAControlPlane resources — not ClowdApps or Deployments. Poll the CAPI Cluster's status.conditions[type=Ready] which aggregates readiness from the child resources, matching how bonfire's ocviapy-based watcher handles them. Also prevent vacuous success when no resources exist yet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@bonfire_lib/deploy.py`:
- Around line 208-209: When list API calls fail during resource readiness
checks, the `all_ready` flag remains True, allowing the function to return
success despite a failed check. Wrap all list operations that occur within the
readiness checking loop with error handling that sets `all_ready = False` when a
list call fails or throws an exception. This applies to all list call locations
in the loop where resources are being checked for readiness status, ensuring
that any failed API call properly marks the readiness check as incomplete.
- Around line 138-145: The `_parse_template` function returns the result of
`yaml.safe_load()` without validating its type. Since `yaml.safe_load()` can
return None, a list, or scalar values instead of a dictionary, you need to add
validation after the successful parse to ensure the returned value is a
dictionary. If it's not a dictionary, raise a FatalError with a message
indicating that the template content must be a valid YAML dictionary/object, not
providing a less actionable error message later in the code.
In `@bonfire_lib/qontract.py`:
- Around line 180-184: The int() calls on the param values from
this_params.get() and other_params.get() in the loop over REPLICAS and
MIN_REPLICAS will raise an exception if the values are non-numeric (templated or
malformed), causing the entire get_apps_for_env function to fail. Wrap the int()
conversions in a try-except block or use a safe conversion helper function to
handle non-numeric values gracefully, allowing the weight calculation to
continue without crashing when encountering templated or malformed replica
values.
In `@bonfire_lib/repo_fetch.py`:
- Line 41: The regex pattern GIT_SHA_RE using re.match with [a-f0-9]{40} only
validates that a string starts with 40 hex characters, not that the entire
string is exactly 40 hex characters. This can misclassify invalid refs as valid
SHAs. Fix this by either adding anchors (^ at the start and $ at the end) to the
regex pattern to enforce exact matching, or by using re.fullmatch instead of
re.match in the code that checks against GIT_SHA_RE. Apply this fix to all three
occurrences mentioned in the comment.
- Around line 184-193: The rate-limit header parsing logic in the conditional
blocks is not robust against missing or invalid header values. When parsing the
retry-after header or x-ratelimit-reset header, wrap the int() conversions in
try-except blocks to handle ValueError exceptions gracefully. Additionally,
ensure the computed sleep_seconds value is never negative by using max(0,
sleep_seconds) after calculation, and provide sensible default values (like 60
seconds) when headers are missing or parsing fails. This prevents the fetch
operation from crashing due to malformed rate-limit headers and ensures the
retry logic completes successfully with appropriate wait times.
In `@tests/test_bonfire_lib/test_deploy.py`:
- Line 4: Remove the unused `call` import from the import statement on line 4 in
the test_deploy.py module. The `call` symbol is imported from unittest.mock but
is never used anywhere in the file, which triggers the F401 lint error. Update
the import statement to only include MagicMock and patch from unittest.mock.
In `@tests/test_bonfire_mcp/test_server.py`:
- Line 4: The import statement from unittest.mock includes AsyncMock but this
import is not used anywhere in the test file. Remove AsyncMock from the import
statement at the top of the file while keeping the other imports like MagicMock
and patch that are actually used in the file.
🪄 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: Enterprise
Run ID: a398fd5c-85b8-47e6-abd0-a7c3f1dc7cc6
📒 Files selected for processing (18)
bonfire_lib/__init__.pybonfire_lib/clusters.pybonfire_lib/core_resources.pybonfire_lib/deploy.pybonfire_lib/k8s_client.pybonfire_lib/pools.pybonfire_lib/qontract.pybonfire_lib/repo_fetch.pybonfire_lib/reservations.pybonfire_lib/templates/clusterreservation.yaml.j2bonfire_mcp/formatters.pybonfire_mcp/server.pypyproject.tomltests/test_bonfire_lib/test_deploy.pytests/test_bonfire_lib/test_qontract.pytests/test_bonfire_lib/test_repo_fetch.pytests/test_bonfire_mcp/test_formatters.pytests/test_bonfire_mcp/test_server.py
💤 Files with no reviewable changes (3)
- bonfire_lib/templates/clusterreservation.yaml.j2
- bonfire_lib/clusters.py
- bonfire_lib/core_resources.py
Comment on lines
+138
to
+145
| def _parse_template(component_name: str, content: bytes) -> dict: | ||
| """Parse template YAML content.""" | ||
| try: | ||
| return yaml.safe_load(content) | ||
| except Exception as err: | ||
| raise FatalError( | ||
| f"failed to parse template YAML for component '{component_name}': {err}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Validate parsed template structure before returning.
yaml.safe_load can return None/list/scalar without raising. That currently fails later with a less actionable exception path.
Proposed fix
def _parse_template(component_name: str, content: bytes) -> dict:
"""Parse template YAML content."""
try:
- return yaml.safe_load(content)
+ parsed = yaml.safe_load(content)
except Exception as err:
raise FatalError(
f"failed to parse template YAML for component '{component_name}': {err}"
)
+ if not isinstance(parsed, dict):
+ raise FatalError(
+ f"template YAML for component '{component_name}' did not parse to an object"
+ )
+ return parsed🤖 Prompt for 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.
In `@bonfire_lib/deploy.py` around lines 138 - 145, The `_parse_template` function
returns the result of `yaml.safe_load()` without validating its type. Since
`yaml.safe_load()` can return None, a list, or scalar values instead of a
dictionary, you need to add validation after the successful parse to ensure the
returned value is a dictionary. If it's not a dictionary, raise a FatalError
with a message indicating that the template content must be a valid YAML
dictionary/object, not providing a less actionable error message later in the
code.
Comment on lines
+208
to
+209
| found_resources = False | ||
|
|
Contributor
There was a problem hiding this comment.
Do not treat list errors as readiness success.
When list calls fail, all_ready stays True; combined with sticky found_resources, the loop can return success without a successful readiness check in the current poll.
Proposed fix
- found_resources = False
-
while True:
elapsed = time.time() - start
@@
- all_ready = True
+ all_ready = True
+ found_resources = False
@@
except Exception as err:
log.debug("error listing CAPI Clusters: %s", err)
+ all_ready = False
@@
except Exception as err:
log.debug("error listing ClowdApps: %s", err)
+ all_ready = False
@@
except Exception as err:
log.debug("error listing Deployments: %s", err)
+ all_ready = FalseAlso applies to: 231-233, 246-248, 261-266
🤖 Prompt for 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.
In `@bonfire_lib/deploy.py` around lines 208 - 209, When list API calls fail
during resource readiness checks, the `all_ready` flag remains True, allowing
the function to return success despite a failed check. Wrap all list operations
that occur within the readiness checking loop with error handling that sets
`all_ready = False` when a list call fails or throws an exception. This applies
to all list call locations in the loop where resources are being checked for
readiness status, ensuring that any failed API call properly marks the readiness
check as incomplete.
Comment on lines
+180
to
+184
| for param_name in ("REPLICAS", "MIN_REPLICAS"): | ||
| if int(this_params.get(param_name, 0)) >= 1: | ||
| this_weight += 1 | ||
| if int(other_params.get(param_name, 0)) >= 1: | ||
| other_weight += 1 |
Contributor
There was a problem hiding this comment.
Guard replica weight parsing against non-numeric values.
int() on malformed/templated replica values will raise and fail get_apps_for_env for the entire env.
Proposed fix
for param_name in ("REPLICAS", "MIN_REPLICAS"):
- if int(this_params.get(param_name, 0)) >= 1:
+ try:
+ this_replicas = int(this_params.get(param_name, 0))
+ except (TypeError, ValueError):
+ this_replicas = 0
+ try:
+ other_replicas = int(other_params.get(param_name, 0))
+ except (TypeError, ValueError):
+ other_replicas = 0
+ if this_replicas >= 1:
this_weight += 1
- if int(other_params.get(param_name, 0)) >= 1:
+ if other_replicas >= 1:
other_weight += 1📝 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
| for param_name in ("REPLICAS", "MIN_REPLICAS"): | |
| if int(this_params.get(param_name, 0)) >= 1: | |
| this_weight += 1 | |
| if int(other_params.get(param_name, 0)) >= 1: | |
| other_weight += 1 | |
| for param_name in ("REPLICAS", "MIN_REPLICAS"): | |
| try: | |
| this_replicas = int(this_params.get(param_name, 0)) | |
| except (TypeError, ValueError): | |
| this_replicas = 0 | |
| try: | |
| other_replicas = int(other_params.get(param_name, 0)) | |
| except (TypeError, ValueError): | |
| other_replicas = 0 | |
| if this_replicas >= 1: | |
| this_weight += 1 | |
| if other_replicas >= 1: | |
| other_weight += 1 |
🤖 Prompt for 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.
In `@bonfire_lib/qontract.py` around lines 180 - 184, The int() calls on the param
values from this_params.get() and other_params.get() in the loop over REPLICAS
and MIN_REPLICAS will raise an exception if the values are non-numeric
(templated or malformed), causing the entire get_apps_for_env function to fail.
Wrap the int() conversions in a try-except block or use a safe conversion helper
function to handle non-numeric values gracefully, allowing the weight
calculation to continue without crashing when encountering templated or
malformed replica values.
|
|
||
| GL_CA_CERT_URL = "https://certs.corp.redhat.com/certs/2022-IT-Root-CA.pem" | ||
|
|
||
| GIT_SHA_RE = re.compile(r"[a-f0-9]{40}") |
Contributor
There was a problem hiding this comment.
Use strict SHA matching before skipping ref resolution.
re.match with [a-f0-9]{40} accepts strings that only start with 40 hex chars. That can misclassify invalid refs as SHAs.
Proposed fix
-GIT_SHA_RE = re.compile(r"[a-f0-9]{40}")
+GIT_SHA_RE = re.compile(r"[a-f0-9]{40}")
...
- if not GIT_SHA_RE.match(commit):
+ if not GIT_SHA_RE.fullmatch(commit):
commit = self._get_gh_commit_hash()
...
- if not GIT_SHA_RE.match(commit):
+ if not GIT_SHA_RE.fullmatch(commit):
commit = self._get_gl_commit_hash()Also applies to: 219-220, 270-271
🤖 Prompt for 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.
In `@bonfire_lib/repo_fetch.py` at line 41, The regex pattern GIT_SHA_RE using
re.match with [a-f0-9]{40} only validates that a string starts with 40 hex
characters, not that the entire string is exactly 40 hex characters. This can
misclassify invalid refs as valid SHAs. Fix this by either adding anchors (^ at
the start and $ at the end) to the regex pattern to enforce exact matching, or
by using re.fullmatch instead of re.match in the code that checks against
GIT_SHA_RE. Apply this fix to all three occurrences mentioned in the comment.
Comment on lines
+184
to
+193
| if "retry-after" in response.headers: | ||
| sleep_seconds = int(response.headers["retry-after"]) | ||
| elif response.headers.get("x-ratelimit-remaining") == "0": | ||
| reset_time = ( | ||
| int(response.headers["x-ratelimit-reset"]) | ||
| or time.time() + 60 | ||
| ) | ||
| sleep_seconds = reset_time - time.time() | ||
| else: | ||
| sleep_seconds = 60 |
Contributor
There was a problem hiding this comment.
Harden rate-limit header parsing to avoid retry-path crashes.
x-ratelimit-reset and retry-after are parsed as mandatory integers; missing/invalid values (or negative computed delays) can raise and abort fetch instead of retrying.
Proposed fix
- if "retry-after" in response.headers:
- sleep_seconds = int(response.headers["retry-after"])
+ if "retry-after" in response.headers:
+ try:
+ sleep_seconds = int(response.headers["retry-after"])
+ except (TypeError, ValueError):
+ sleep_seconds = 60
elif response.headers.get("x-ratelimit-remaining") == "0":
- reset_time = (
- int(response.headers["x-ratelimit-reset"])
- or time.time() + 60
- )
- sleep_seconds = reset_time - time.time()
+ try:
+ reset_time = int(response.headers.get("x-ratelimit-reset", 0))
+ except (TypeError, ValueError):
+ reset_time = 0
+ if reset_time <= 0:
+ reset_time = int(time.time()) + 60
+ sleep_seconds = max(0, reset_time - int(time.time()))
else:
sleep_seconds = 60🤖 Prompt for 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.
In `@bonfire_lib/repo_fetch.py` around lines 184 - 193, The rate-limit header
parsing logic in the conditional blocks is not robust against missing or invalid
header values. When parsing the retry-after header or x-ratelimit-reset header,
wrap the int() conversions in try-except blocks to handle ValueError exceptions
gracefully. Additionally, ensure the computed sleep_seconds value is never
negative by using max(0, sleep_seconds) after calculation, and provide sensible
default values (like 60 seconds) when headers are missing or parsing fails. This
prevents the fetch operation from crashing due to malformed rate-limit headers
and ensures the retry logic completes successfully with appropriate wait times.
| """Tests for bonfire_lib.deploy module.""" | ||
|
|
||
| import pytest | ||
| from unittest.mock import MagicMock, patch, call |
Contributor
There was a problem hiding this comment.
Remove unused call import to satisfy lint checks.
Line 4 imports call but this symbol is never used in the module, which triggers F401.
Suggested fix
-from unittest.mock import MagicMock, patch, call
+from unittest.mock import MagicMock, patch📝 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
| from unittest.mock import MagicMock, patch, call | |
| from unittest.mock import MagicMock, patch |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 4-4: 'unittest.mock.call' imported but unused
(F401)
🤖 Prompt for 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.
In `@tests/test_bonfire_lib/test_deploy.py` at line 4, Remove the unused `call`
import from the import statement on line 4 in the test_deploy.py module. The
`call` symbol is imported from unittest.mock but is never used anywhere in the
file, which triggers the F401 lint error. Update the import statement to only
include MagicMock and patch from unittest.mock.
Source: Linters/SAST tools
|
|
||
| import pytest | ||
| from unittest.mock import MagicMock, patch | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
Contributor
There was a problem hiding this comment.
Remove unused AsyncMock import.
Static analysis indicates AsyncMock is imported but never used in this file.
-from unittest.mock import AsyncMock, MagicMock, patch
+from unittest.mock import MagicMock, patch📝 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
| from unittest.mock import AsyncMock, MagicMock, patch | |
| from unittest.mock import MagicMock, patch |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 4-4: 'unittest.mock.AsyncMock' imported but unused
(F401)
🤖 Prompt for 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.
In `@tests/test_bonfire_mcp/test_server.py` at line 4, The import statement from
unittest.mock includes AsyncMock but this import is not used anywhere in the
test file. Remove AsyncMock from the import statement at the top of the file
while keeping the other imports like MagicMock and patch that are actually used
in the file.
Source: Linters/SAST tools
Extracts the MCP server into its own publishable package (crc-bonfire-mcp) with a dedicated pyproject.toml and Dockerfile-mcp, so consumers only pull in MCP+bonfire_lib dependencies without the full CLI stack. Also fixes process_template to use the namespaced processedtemplates endpoint and improves MCP server startup error handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bsquizz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile-mcp (1)
17-17: ⚡ Quick winCNB_USER_ID and actual UID mismatch.
Line 17 sets
CNB_USER_ID=1000, but line 24 creates the user with UID1001. This inconsistency is confusing. Based on the error message inbonfire_mcp/server.py("--userns=keep-id:uid=1000,gid=1000"), it appearsCNB_USER_ID=1000is intentional for Podman user namespace mapping, but the actual container user is1001.If this mismatch is required for compatibility, consider adding a comment explaining the relationship between
CNB_USER_ID(external mapping hint) and the actual internal UID.📝 Suggested clarifying comment
+# CNB_USER_ID is a hint for external tooling (e.g., podman --userns=keep-id:uid=1000). +# The actual container user runs as UID 1001. ENV PYTHON_VERSION=3.12 \ PATH=$APP_ROOT/.local/bin/:$PATH \ PYTHONUNBUFFERED=1 \ PYTHONIOENCODING=UTF-8 \ LANG=en_US.UTF-8 \ CNB_USER_ID=1000 \ CNB_GROUP_ID=0 \ PIP_NO_CACHE_DIR=offAlso applies to: 24-26
🤖 Prompt for 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. In `@Dockerfile-mcp` at line 17, Add a clarifying comment in the Dockerfile explaining the intentional mismatch between CNB_USER_ID=1000 set on line 17 and the actual container user UID of 1001 created around line 24. The comment should clarify that CNB_USER_ID=1000 is used as an external mapping hint for Podman user namespace mapping (referenced in bonfire_mcp/server.py with the --userns=keep-id:uid=1000,gid=1000 flag), while the actual internal container user runs with UID 1001. This will prevent confusion about why these values differ and document the intentional compatibility requirement.Dockerfile (1)
23-23: ⚡ Quick winCNB_USER_ID and actual UID mismatch (same as Dockerfile-mcp).
Line 23 now sets
CNB_USER_ID=1000(changed from1001), but line 30 still creates the user with UID1001. This creates the same inconsistency present inDockerfile-mcp.The alignment across both Dockerfiles suggests this is intentional for Podman user namespace mapping consistency, but it remains confusing without inline documentation.
📝 Suggested clarifying comment
+# CNB_USER_ID is a hint for external tooling (e.g., podman --userns=keep-id:uid=1000). +# The actual container user runs as UID 1001. ENV PYTHON_VERSION=3.12 \ PATH=$APP_ROOT/.local/bin/:$PATH \ PYTHONUNBUFFERED=1 \ PYTHONIOENCODING=UTF-8 \ LANG=en_US.UTF-8 \ CNB_USER_ID=1000 \ CNB_GROUP_ID=0 \ PIP_NO_CACHE_DIR=offAlso applies to: 30-30
🤖 Prompt for 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. In `@Dockerfile` at line 23, The CNB_USER_ID environment variable on line 23 is set to 1000, but the actual user creation on line 30 uses UID 1001, creating an inconsistency. Either change the CNB_USER_ID value to match the user creation UID of 1001, or change the user creation command to use UID 1000 to match CNB_USER_ID. If this mismatch is intentional for Podman user namespace mapping consistency, add an inline comment near both occurrences explaining the deliberate difference to prevent future confusion.
🤖 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 `@bonfire_mcp/pyproject.toml`:
- Around line 38-39: The setuptools_scm configuration in pyproject.toml is set
to look for version tags in the parent directory with root = ".." but the
repository currently has no git tags defined. To fix this, create annotated git
tags at the repository root (not in the bonfire_mcp subdirectory) using a
standard versioning format such as v1.0.0. Once the git tags are created at the
repository root level, setuptools_scm will be able to discover them when
building wheels from the bonfire_mcp subdirectory, and the wheel will use the
proper version string instead of falling back to a dev version like
0.0.dev0+g<commit_hash>.
---
Nitpick comments:
In `@Dockerfile`:
- Line 23: The CNB_USER_ID environment variable on line 23 is set to 1000, but
the actual user creation on line 30 uses UID 1001, creating an inconsistency.
Either change the CNB_USER_ID value to match the user creation UID of 1001, or
change the user creation command to use UID 1000 to match CNB_USER_ID. If this
mismatch is intentional for Podman user namespace mapping consistency, add an
inline comment near both occurrences explaining the deliberate difference to
prevent future confusion.
In `@Dockerfile-mcp`:
- Line 17: Add a clarifying comment in the Dockerfile explaining the intentional
mismatch between CNB_USER_ID=1000 set on line 17 and the actual container user
UID of 1001 created around line 24. The comment should clarify that
CNB_USER_ID=1000 is used as an external mapping hint for Podman user namespace
mapping (referenced in bonfire_mcp/server.py with the
--userns=keep-id:uid=1000,gid=1000 flag), while the actual internal container
user runs with UID 1001. This will prevent confusion about why these values
differ and document the intentional compatibility requirement.
🪄 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: Enterprise
Run ID: 53e4e6aa-52fc-48bc-8993-82da2a834153
📒 Files selected for processing (10)
.gitignoreDockerfileDockerfile-mcpREADME.mdbonfire_lib/deploy.pybonfire_lib/k8s_client.pybonfire_mcp/README.mdbonfire_mcp/pyproject.tomlbonfire_mcp/server.pypyproject.toml
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bonfire_lib/deploy.py
- bonfire_mcp/server.py
- bonfire_lib/k8s_client.py
Comment on lines
+38
to
+39
| [tool.setuptools_scm] | ||
| root = ".." |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify setuptools_scm can find version tags from subdirectory
cd bonfire_mcp
python3 -c "from setuptools_scm import get_version; print(get_version(root='..', version_scheme='post-release'))"Repository: RedHatInsights/bonfire
Length of output: 194
🏁 Script executed:
# Check git tags in the repository
git tag -l | head -20Repository: RedHatInsights/bonfire
Length of output: 48
🏁 Script executed:
# Check the current directory structure
pwd
ls -laRepository: RedHatInsights/bonfire
Length of output: 1755
🏁 Script executed:
# Read the bonfire_mcp/pyproject.toml file to see the full configuration
cat bonfire_mcp/pyproject.tomlRepository: RedHatInsights/bonfire
Length of output: 1416
🏁 Script executed:
# Explicitly list git tags
git tag --list
git describe --tags --alwaysRepository: RedHatInsights/bonfire
Length of output: 74
🏁 Script executed:
# Read the root pyproject.toml to see setuptools_scm configuration there
cat pyproject.tomlRepository: RedHatInsights/bonfire
Length of output: 2298
🏁 Script executed:
# Check if .git directory contains any tags
git rev-list --all --tagsRepository: RedHatInsights/bonfire
Length of output: 32825
🏁 Script executed:
# List all git tags in the repository
git tag --list 2>&1 | head -50
git tag --list --sort=-version:refname 2>&1 | head -20Repository: RedHatInsights/bonfire
Length of output: 48
🏁 Script executed:
# Check if there are any annotated tags
git tag -n1 2>&1 | head -20Repository: RedHatInsights/bonfire
Length of output: 48
🏁 Script executed:
# Check what setuptools_scm would discover from parent directory
git log --oneline --decorate -20Repository: RedHatInsights/bonfire
Length of output: 1431
Add git version tags to repository before building wheels.
The repository contains no git tags (git tag --list returns empty). When setuptools_scm attempts to discover versions with root = "..", it will fail to find tags and fall back to generating a dev version like 0.0.dev0+g<commit_hash>. This will cause the wheel build to use an unintended version string or potentially fail.
Create annotated git tags (e.g., v1.0.0) at the repository root so setuptools_scm can discover them from the bonfire_mcp subdirectory.
🤖 Prompt for 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.
In `@bonfire_mcp/pyproject.toml` around lines 38 - 39, The setuptools_scm
configuration in pyproject.toml is set to look for version tags in the parent
directory with root = ".." but the repository currently has no git tags defined.
To fix this, create annotated git tags at the repository root (not in the
bonfire_mcp subdirectory) using a standard versioning format such as v1.0.0.
Once the git tags are created at the repository root level, setuptools_scm will
be able to discover them when building wheels from the bonfire_mcp subdirectory,
and the wheel will use the proper version string instead of falling back to a
dev version like 0.0.dev0+g<commit_hash>.
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
MCP refactor: unify cluster/namespace into pool-based dispatch
bonfire_lib/clusters.pyand theClusterReservationCRD approach — cluster reservations now go through the same namespace reservation pool mechanismbonfire_lib/core_resources.pyandbonfire_lib/templates/clusterreservation.yaml.j2bonfire_lib/pools.pyto remove cluster-specific pool logicbonfire_lib/reservations.pywith unifiedreserve(),release(),extend()functionsbonfire_mcp/server.pyto use a single pool-based tool dispatch modelbonfire_mcp/formatters.pyand its testsephemeral_deploy_rosaMCP toolMove deploy_ephemeral_rosa from CLI subprocess to bonfire_lib
asyncio.create_subprocess_exec("bonfire", "deploy", "rosa", ...)call with pure Python library calls throughbonfire_lib, eliminating the dependency on theocbinary,ocviapy, andshfor MCP consumersbonfire_libmodules:qontract.py— app-interface GraphQL clientrepo_fetch.py— HTTP template fetching from GitHub/GitLabdeploy.py— deploy orchestration with readiness pollingEphemeralK8sClientwithapply_resource(),list_dynamic_resources(), andprocess_template()(OpenShift Template API) methodsSplit bonfire_mcp into standalone package
bonfire_mcp/pyproject.tomlto publish the MCP server as a separate lightweight package (crc-bonfire-mcp) that only pulls inkubernetes,gql[requests],PyYAML,jinja2, andmcp— no CLI dependencies (click,ocviapy,rich, etc.)Dockerfile-mcpfor building a dedicated MCP server container imagebonfire-mcpentrypoint andmcp/liboptional extras from the maincrc-bonfirepackagebonfire_mcpfrom main package discoveryBug fixes
process_template()to use the namespaced processedtemplates API endpoint (/namespaces/{ns}/processedtemplates) and correctresponse_types_mapwait_for_resources()to poll CAPIClusterresources (cluster.x-k8s.io/v1beta1) viastatus.conditions[type=Ready]Documentation
crc-bonfire-mcppackagebonfire_mcp/README.mdinto the mainREADME.md--userns)Details
The
deploy_rosaflow:bonfire_lib.reservations)bonfire_lib.qontract)bonfire_lib.repo_fetch)EphemeralK8sClient.process_template)EphemeralK8sClient.apply_resource)bonfire_lib.deploy.wait_for_resources) — polls CAPICluster(cluster.x-k8s.io/v1beta1) resources viastatus.conditions[type=Ready]bonfire_lib.status)On deploy failure, the reserved namespace is automatically released.
Test plan
test_qontract.py,test_repo_fetch.py,test_deploy.pyTestDeployRosa) — mocksbonfire_lib.deployinstead of subprocesssubprocess,ocviapy, orshimports inbonfire_lib/orbonfire_mcp/🤖 Generated with Claude Code