Conversation
Creates a bootc based image based on MicroShift for evaluation and small labs.
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAdds a MicroShift BootC image build/runtime system and a new Flask-based configuration service (API, UI, auth, system utilities), container/build tooling, systemd units, templates, and documentation to enable local MicroShift bootstrap, image/ISO generation, and in-container Jumpstarter management. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User/Browser
participant UI as Config UI
participant API as Flask API
participant Sys as System Utils
participant K8s as Kubernetes/oc
Browser->>UI: Load admin page
UI->>API: GET /api/system-stats
API->>Sys: collect disk/memory/cpu/network/LVM
Sys-->>API: metrics JSON
API-->>UI: Render system stats
Browser->>UI: Submit configure-jumpstarter
UI->>API: POST /api/configure-jumpstarter
API->>Sys: validate_hostname -> apply_jumpstarter_cr
Sys->>K8s: oc apply (using kubeconfig)
K8s-->>Sys: apply result
Sys-->>API: success/failure
API-->>UI: Display result
Browser->>UI: View pods/routes
UI->>API: GET /api/pods or /api/routes
API->>K8s: oc get pods/routes (kubeconfig)
K8s-->>API: resource list
API-->>UI: Render tables
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
📝 Coding Plan
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 |
![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: 18
🧹 Nitpick comments (11)
controller/deploy/microshift-bootc/config-svc/app.py (2)
16-16: Minor typo in comment."registrationfunctions" should be "registration functions".
-# Import route registrationfunctions +# Import route registration functions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/app.py` at line 16, Fix the typo in the inline comment that reads "registrationfunctions" by updating it to "registration functions" so the comment correctly reads something like "Import route registration functions"; locate the comment near the import block in app.py that currently contains "registrationfunctions" and replace the single-word typo with the two-word phrase.
31-31: Consider changing the default port to match deployment configuration.The default port is
8080, but the Containerfile exposes port8880andrun-microshift.shmaps8880:8880. While the systemd service (config-svc.service) overrides this withEnvironment="PORT=8880", changing the default to8880improves consistency for local development and reduces confusion:Proposed change
- port = int(os.environ.get('PORT', 8080)) + port = int(os.environ.get('PORT', 8880))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/app.py` at line 31, The default port for the app is set to 8080 (port = int(os.environ.get('PORT', 8080))) which conflicts with the Containerfile and run scripts that use 8880; update the default value to 8880 so that the PORT fallback matches deployment and local dev (i.e., change the default in the PORT getenv call from 8080 to 8880 in the app.py port assignment).controller/deploy/microshift-bootc/config-svc/templates/password_required.html (1)
85-93: Add defensive checks for API response handling.The response handling assumes
result.messagesis always an array. If the API returns an error response without amessagesfield,result.messages.forEachwill throw. Consider adding a guard:Proposed fix
.then(response => response.json()) .then(result => { // Display messages - result.messages.forEach(msg => { + (result.messages || []).forEach(msg => { const messageDiv = document.createElement('div'); messageDiv.className = `message ${msg.type}`; messageDiv.textContent = msg.text; messagesContainer.appendChild(messageDiv); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/templates/password_required.html` around lines 85 - 93, The code assumes result.messages is always an array and calls result.messages.forEach which will throw if messages is missing or not an array; update the response handling (the promise chain that processes response.json and the block referencing result, result.messages, messagesContainer, msg.type, msg.text) to defensively check that Array.isArray(result.messages) (or that result.messages is truthy and iterable) before iterating, and handle the else case by rendering a default error message or skipping rendering so the UI doesn't crash.controller/deploy/microshift-bootc/Containerfile (1)
19-20: Document security implications of default password.The hardcoded default password
"jumpstarter"is intentional and correctly triggers the password change flow viais_default_password()inauth.py. Consider adding a comment explaining this is by design and that users are forced to change it on first access.Proposed documentation
-# Set root password +# Set default root password - users are forced to change this on first login +# via the password_required.html flow (see auth.py:is_default_password()) RUN echo "root:jumpstarter" | chpasswd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/Containerfile` around lines 19 - 20, Add an explanatory comment above the RUN echo "root:jumpstarter" | chpasswd line stating that the hardcoded default password "jumpstarter" is intentional for initial boot, that the auth.py is_default_password() check forces users to change it on first login, and note the security implications so reviewers/users understand this is deliberate and must not be used in production without changing; reference the is_default_password() function in auth.py in the comment to tie behavior to implementation.controller/deploy/microshift-bootc/run-microshift.sh (1)
86-86: DRY: Extract hostname to a single constant.The hostname
"jumpstarter.127-0-0-1.nip.io"is defined twice (line 86 and line 154). Consider defining it once at the top with the other constants to avoid inconsistency if one is updated without the other.Proposed fix
CONFIG_SVC_PORT=8880 +HOSTNAME="jumpstarter.127-0-0-1.nip.io" function pull_bootc_image() {Then use
${HOSTNAME}in both locations.Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/run-microshift.sh` at line 86, Extract the duplicate literal "jumpstarter.127-0-0-1.nip.io" into a single read-only constant (e.g., HOSTNAME) alongside the other top-of-file constants in run-microshift.sh, then replace the two inline definitions/usages (the current local -r hostname="..." and the other occurrence) with references to ${HOSTNAME}; ensure you remove the duplicated local -r hostname declaration so both places use the single HOSTNAME constant.controller/deploy/microshift-bootc/Makefile (2)
1-1: Addcleanto.PHONYdeclaration.The static analysis tool correctly identified that
cleanis missing from the.PHONYdeclaration. While this won't break functionality, it's a Makefile best practice.♻️ Proposed fix
-.PHONY: help build bootc-build bootc-build-multi push bootc-push bootc-push-multi bootc-run bootc-stop bootc-sh bootc-rm build-image build-iso build-all build-all-multi push-all push-all-multi +.PHONY: help build bootc-build bootc-build-multi push bootc-push bootc-push-multi bootc-run bootc-stop bootc-sh bootc-rm build-image build-iso build-all build-all-multi push-all push-all-multi clean bootc-reload-app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/Makefile` at line 1, The .PHONY declaration at the top of the Makefile is missing the clean target; update the .PHONY line (the line containing ".PHONY: help build bootc-build ...") to include "clean" so the clean target is treated as phony and not confused with a file of the same name—add "clean" to that list.
76-92: LVM cleanup uses hardcoded volume group name.The cleanup commands use hardcoded
myvg1for the volume group name. If the VG name changes or differs across environments, these cleanup steps will be ineffective.♻️ Consider parameterizing the VG name
+# LVM configuration +LVM_VG_NAME ?= myvg1 + build-image: bootc-build ## Build the bootc based qcow2 image `@echo` "Building image: output/qcow2/disk.qcow2" `@echo` "Cleaning up any existing LVM resources to avoid conflicts..." - -sudo vgs --noheadings -o vg_name,vg_uuid | grep myvg1 | while read vg uuid; do sudo vgremove -f --select vg_uuid=$$uuid 2>/dev/null || true; done + -sudo vgs --noheadings -o vg_name,vg_uuid | grep $(LVM_VG_NAME) | while read vg uuid; do sudo vgremove -f --select vg_uuid=$$uuid 2>/dev/null || true; done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/Makefile` around lines 76 - 92, The LVM cleanup in the build-image and build-iso targets hardcodes the VG name "myvg1"; change it to use a Makefile variable (e.g., VG_NAME ?= myvg1) and reference that variable in the vgs/grep/vgremove commands so different environments can override it (update the commands in the build-image and build-iso targets that currently call vgs ... | grep myvg1 to use $(VG_NAME)); ensure the variable has a sensible default and that both targets are updated consistently.controller/deploy/microshift-bootc/config-svc/api.py (3)
702-730: Log streaming process cleanup could leave zombie processes.In the
generate()function, if the client disconnects abruptly, thefinallyblock may not execute immediately due to how Flask generators work. Consider adding a timeout toprocess.wait().♻️ Add more robust process cleanup
finally: # Clean up process when connection closes if process: try: process.terminate() - process.wait(timeout=5) + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + process.wait(timeout=1) except Exception: process.kill() + try: + process.wait(timeout=1) + except Exception: + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/api.py` around lines 702 - 730, The generate() generator can leave zombie oc processes when a client disconnects; update the subprocess.Popen call to start the process in its own session (use start_new_session=True or preexec_fn=os.setsid) and in the finally block call process.terminate() then process.wait(timeout=5) inside a try/except that on TimeoutExpired uses process.kill() and, if you started a session, os.killpg(process.pid, signal.SIGKILL) to ensure the whole group is killed; ensure these changes reference the generate function, the subprocess.Popen invocation, and the process variable so cleanup always forcibly reaps the child.
390-393: Image validation is too permissive.The validation check
':' in image or '/' in imagewill accept many invalid formats. For example,foo:or/invalidwould pass. Consider using a more robust validation pattern.♻️ Proposed stricter validation
- # Validate image format (basic check) - if not (image.startswith('quay.io/') or image.startswith('docker.io/') or - ':' in image or '/' in image): - return jsonify({'success': False, 'error': 'Invalid image reference format'}), 400 + # Validate image format (basic check for registry/repo:tag pattern) + # Must contain at least one '/' for namespace/repo and optionally ':' for tag + image_pattern = re.compile(r'^[\w.-]+(/[\w.-]+)+(:[\w.-]+)?$') + if not image_pattern.match(image): + return jsonify({'success': False, 'error': 'Invalid image reference format. Expected format: registry/namespace/repo:tag'}), 400🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/api.py` around lines 390 - 393, The current check around the image variable in the validation block is too permissive (the clause "':' in image or '/' in image") and will allow invalid values like "foo:" or "/invalid"; replace that ad-hoc check with a strict regex-based validation for container image references (match optional registry host[:port], one or more path components for namespace/repo, image name, and an optional :tag or `@digest`) and use re.match on image before returning the jsonify error; update the condition in the same validation block (the code that currently returns jsonify({'success': False, 'error': 'Invalid image reference format'}), 400) to use the compiled regex (or a helper function is_valid_image(image)) so only syntactically correct image references are accepted.
831-855: Kubeconfig regex manipulation is fragile and assumes specific YAML format.The regex replacements for modifying the kubeconfig assume a specific YAML structure (specific indentation, field ordering, line endings). MicroShift's kubeconfig format is unlikely to change, but these replacements could fail silently if the format does change, resulting in an invalid or incomplete kubeconfig.
Consider parsing the kubeconfig as YAML, modifying the data structure, and serializing it back. This would be more robust and format-agnostic:
import yaml config = yaml.safe_load(kubeconfig_content) # Modify clusters[0].cluster.server, remove insecure-skip-tls-verify, add tls-server-name kubeconfig_content = yaml.dump(config)PyYAML is not currently a dependency, but adding it would make this code more maintainable and resilient to format variations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/api.py` around lines 831 - 855, The current sequence of regex edits on kubeconfig_content (replacing server host, removing insecure-skip-tls-verify, and inserting tls-server-name using nip_hostname and original_hostname) is fragile; instead parse kubeconfig_content as YAML (e.g., yaml.safe_load), update the cluster entries programmatically: set each cluster.cluster.server to use nip_hostname, remove any insecure-skip-tls-verify keys under cluster entries, and add tls-server-name: original_hostname under the same cluster.cluster mapping, then serialize back with yaml.safe_dump (yaml.dump) to kubeconfig_content; add PyYAML as a dependency and ensure you operate on all clusters/contexts rather than relying on regex/idempotent string replacements.controller/deploy/microshift-bootc/config-svc/system.py (1)
348-368: Custom YAML serializer produces non-standard formatting for list items with nested objects.The current
json_to_yaml()implementation outputs list items with the dash (-) on its own line, followed by the nested content indented below:endpoints: - address: test.example.comStandard YAML formatting places the first key-value pair on the same line as the dash:
endpoints: - address: test.example.comWhile both formats are valid YAML, the standard format is more conventional and readable.
♻️ Proposed fix
Modify the list handling to place the first key-value pair on the same line as the dash:
elif isinstance(obj, list): for item in obj: - if isinstance(item, (dict, list)): - lines.append(f"{indent_str}-") - lines.append(json_to_yaml(item, indent + 1)) + if isinstance(item, dict): + # First key-value on same line as dash + first = True + for key, value in item.items(): + if first: + if isinstance(value, (dict, list)): + lines.append(f"{indent_str}- {key}:") + lines.append(json_to_yaml(value, indent + 2)) + else: + lines.append(f"{indent_str}- {key}: {yaml_value(value)}") + first = False + else: + if isinstance(value, (dict, list)): + lines.append(f"{indent_str} {key}:") + lines.append(json_to_yaml(value, indent + 2)) + else: + lines.append(f"{indent_str} {key}: {yaml_value(value)}") + elif isinstance(item, list): + lines.append(f"{indent_str}-") + lines.append(json_to_yaml(item, indent + 1)) else: lines.append(f"{indent_str}- {yaml_value(item)}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/config-svc/system.py` around lines 348 - 368, The json_to_yaml function currently writes a lone dash line for list items that are dicts/lists, producing non-standard output; update the list handling in json_to_yaml so when an item is a dict you render the dash and the first key/value on the same line (e.g., "- key: value") and then append remaining nested lines indented one extra level; locate the branch in json_to_yaml where it detects isinstance(item, (dict, list)) and change it to serialize the first line of the nested dict inline after the dash (using yaml_value for simple scalars) and then call json_to_yaml for the rest of the nested content with indent+1, ensuring subsequent lines are indented correctly (reference json_to_yaml and yaml_value to implement this).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-images.yaml:
- Around line 46-48: The matrix entry uses a full registry+tag in image_name
which conflicts with the tag-generation logic that prefixes ${{ env.REGISTRY }}
and appends version tags; update the image_name value (in the matrix entry
currently set to "quay.io/jumpstarter-dev/microshift/bootc:latest") to the
org/repo format (e.g., "jumpstarter-dev/microshift/bootc") so generated tags
become correct, and add a matching label field (e.g., label: "bootc") so PR
label filtering (build-pr-images/<label>) works; adjust only the image_name and
add label in that matrix item to match the other entries and tagging logic.
In `@controller/deploy/microshift-bootc/config-svc/auth.py`:
- Around line 103-110: Remove the unreliable subprocess-based fallback that
calls subprocess.run(['su', username, '-c', 'true'], ...) for password checking;
since python-pam is guaranteed in the production image, delete that fallback
block from auth.py and make the module rely on the pam-based authentication path
(or raise a clear exception if pam import/initialization fails) so failures are
not silently ignored—ensure the authenticate function (the code path that
currently returns result.returncode == 0) either uses the pam API or surfaces an
explicit error when pam is unavailable.
In `@controller/deploy/microshift-bootc/config-svc/config-svc.service`:
- Around line 4-6: Remove the "Before=getty@.service
systemd-user-sessions.service" ordering from the config-svc unit so local
console login isn't gated on the web UI; update the unit file that contains the
After=network-online.target and Wants=network-online.target lines by deleting
the Before=... entry (leave the banner-updater unit to own the getty ordering
instead).
In `@controller/deploy/microshift-bootc/config-svc/pyproject.toml`:
- Around line 33-34: The console script entry jumpstarter-config-svc points to a
non-existent module app_new; update the entry point to reference the actual
module that defines main(). Either change the pyproject.toml [project.scripts]
value from "app_new:main" to "app:main" to match the existing app.py main()
function, or rename/create module app_new with a main() wrapper; ensure the
symbol main() in app.py (or the new module) is importable under the chosen
module name.
- Around line 46-53: The package name in pyproject.toml is invalid because it
uses a hyphen: update the [tool.setuptools] packages value from "config-svc" to
"config_svc" and adjust [tool.setuptools.package-data] key accordingly so
setuptools matches the actual package name; also fix the faulty entry point
"jumpstarter-config-svc = \"app_new:main\"" to reference the real module and
callable ("app:main") and rename the directory from config-svc to config_svc
(e.g., mv controller/deploy/microshift-bootc/config-svc
controller/deploy/microshift-bootc/config_svc) so package name, package-data and
the entry_point (app:main) are consistent.
In `@controller/deploy/microshift-bootc/config-svc/README.md`:
- Around line 16-32: The fenced project-structure block in the README is missing
a language identifier; update the opening fence for the tree diagram from ``` to
```text so the markdown linter recognizes it as a code block (i.e., change the
project-structure fence in README.md that contains the config-svc/ tree to use
```text).
In `@controller/deploy/microshift-bootc/config-svc/templates/index.html`:
- Around line 783-794: The routes table is vulnerable to XSS because values from
data.routes are interpolated directly into tbody.innerHTML; update the rendering
to escape/encode user-controlled fields (route.namespace, route.name,
route.host, route.service, route.port, route.tls, route.age, and the admitted
badge content used with getAdmittedClass(route.admitted)) before building the
HTML string or, better, create DOM elements programmatically and set
textContent/attributes (e.g., create tr/td nodes, set anchor href via
element.href and text via textContent) instead of using raw template literals
assigned to tbody.innerHTML so all values are safely escaped.
- Around line 740-754: The pod table is vulnerable to XSS because data.pods
values are injected directly into tbody.innerHTML; fix by escaping all dynamic
values before inserting into the DOM (or by setting textContent/attributes
instead of raw HTML). Add an escapeHtml utility and use it for pod.namespace,
pod.name, pod.ready, pod.status, pod.restarts and pod.age when building the row,
use getStatusClass(pod.status) only for the class name (ensure it returns safe
strings), and use encodeURIComponent for URL segments in the /logs link and pass
escaped or text-set values to restartPod(namespace, name). Update the code paths
that build the template (where tbody.innerHTML is assigned) to use the
escaped/encoded values.
- Line 329: The code assigns server error text directly into
messagesContainer.innerHTML using string concatenation (e.g.,
messagesContainer.innerHTML = '<div class="message error">Failed to apply
configuration: ' + error.message + '</div>'), which allows XSS; update these
spots to escape the error output by passing error.message through the existing
escapeHtml function (or create a DOM element and set its textContent) instead of
concatenating raw text, and apply the same change for the other identical
occurrences of messagesContainer.innerHTML assignments so all error.message
insertions use escapeHtml(error.message) or textContent.
In
`@controller/deploy/microshift-bootc/config-svc/templates/password_required.html`:
- Around line 40-45: Add the HTML required attribute to the password input
fields so client-side validation prevents empty submission: update the <input>
with id="newPassword" (name="newPassword") and the <input> with
id="confirmPassword" (name="confirmPassword") to include required (in addition
to existing minlength and type="password") so the form enforces that these
fields must be filled before submit.
In `@controller/deploy/microshift-bootc/config-svc/templates/styles.css`:
- Around line 104-106: The keyframes name fadeIn violates the
keyframes-name-pattern; rename the `@keyframes` identifier from "fadeIn" to
"fade-in" and update the animation declaration that references it (the
animation: fadeIn 0.3s ease-in; rule) to use "fade-in" so the `@keyframes` block
and the animation property match.
In `@controller/deploy/microshift-bootc/config-svc/update-banner.sh`:
- Line 1: The script update-banner.sh has a misleading .sh extension and a
non-portable shebang (#!/bin/env python3); either rename the file to
update-banner.py and update all references in the Containerfile and any callers
to use the new name, or keep the name but replace the shebang with the portable
form (#!/usr/bin/env python3) and ensure the file is executable; update any
Containerfile COPY/ENTRYPOINT lines that reference update-banner.sh accordingly.
In `@controller/deploy/microshift-bootc/kickstart.ks`:
- Around line 10-11: The kickstart currently runs unscoped disk wipes via
zerombr and clearpart --all --initlabel; update the kickstart to target the
intended install device by adding an explicit ignoredisk --only-use=<device> (or
equivalent) and change clearpart --all --initlabel to clearpart
--drives=<device> --initlabel so only the specified device is wiped; ensure the
chosen device name matches what the installer will see and keep zerombr if
needed for label initialization.
- Around line 21-25: The file currently sets an insecure plaintext root password
via the rootpw --plaintext jumpstarter line; replace that with a locked root
account or a secure injection method: remove the plaintext rootpw and enable
rootpw --lock (or provision a unique hashed password or SSH key during
provisioning and set it in %post), and if you must require rotation, expire the
password in the %post section (use rootpw with a hashed value or inject SSH keys
there and then run chage -d 0 or similar to force rotation). Ensure changes
reference the rootpw directive and any %post provisioning logic so no plaintext
root credential remains in the repository.
- Around line 59-60: Replace the mutable image reference used in the
ostreecontainer invocation (the line using
"quay.io/jumpstarter-dev/microshift/bootc:latest") with an immutable, versioned
tag or an image digest (sha256:...) to ensure reproducible Kickstart builds;
update the same pattern wherever the image is referenced (Makefile target,
run-microshift.sh, and the build workflow config) so all consumers use the same
pinned tag/digest and document the chosen version in the repo.
In `@controller/deploy/microshift-bootc/Makefile`:
- Around line 38-39: Remove the stray character in the Makefile echo for the
target output/qcow2/disk.qcow2: in the recipe that echoes the BOOTC image (uses
$(BOOTC_IMG)), delete the trailing 'a' so the message prints only the BOOTC
image path; update the echo line in the Makefile rule for
output/qcow2/disk.qcow2 accordingly.
In `@controller/deploy/microshift-bootc/README.md`:
- Around line 55-63: Update the markdown fenced code blocks under the "Output
example:" section (and the other example block noted) to include a language tag
so the linter stops warning; specifically change the fences from ``` to ```text
for the examples shown in README.md (the "Output example:" block and the later
example around lines mentioned in the review) so both are marked as text-coded
blocks.
In `@controller/deploy/microshift-bootc/run-microshift.sh`:
- Line 172: The echo that prints the Config Service URL uses ${HOSTNAME%%.*}
which yields just "jumpstarter" (non-resolvable); update the echo in
run-microshift.sh to output a usable address instead—either construct the full
nip.io hostname using ${HOSTNAME} and the cluster IP (or a known IP variable) or
simply print http://localhost:${CONFIG_SVC_PORT} (or both) so users see a
resolvable URL; locate the echo referencing HOSTNAME and CONFIG_SVC_PORT and
replace the ${HOSTNAME%%.*} usage with the chosen resolvable host string.
---
Nitpick comments:
In `@controller/deploy/microshift-bootc/config-svc/api.py`:
- Around line 702-730: The generate() generator can leave zombie oc processes
when a client disconnects; update the subprocess.Popen call to start the process
in its own session (use start_new_session=True or preexec_fn=os.setsid) and in
the finally block call process.terminate() then process.wait(timeout=5) inside a
try/except that on TimeoutExpired uses process.kill() and, if you started a
session, os.killpg(process.pid, signal.SIGKILL) to ensure the whole group is
killed; ensure these changes reference the generate function, the
subprocess.Popen invocation, and the process variable so cleanup always forcibly
reaps the child.
- Around line 390-393: The current check around the image variable in the
validation block is too permissive (the clause "':' in image or '/' in image")
and will allow invalid values like "foo:" or "/invalid"; replace that ad-hoc
check with a strict regex-based validation for container image references (match
optional registry host[:port], one or more path components for namespace/repo,
image name, and an optional :tag or `@digest`) and use re.match on image before
returning the jsonify error; update the condition in the same validation block
(the code that currently returns jsonify({'success': False, 'error': 'Invalid
image reference format'}), 400) to use the compiled regex (or a helper function
is_valid_image(image)) so only syntactically correct image references are
accepted.
- Around line 831-855: The current sequence of regex edits on kubeconfig_content
(replacing server host, removing insecure-skip-tls-verify, and inserting
tls-server-name using nip_hostname and original_hostname) is fragile; instead
parse kubeconfig_content as YAML (e.g., yaml.safe_load), update the cluster
entries programmatically: set each cluster.cluster.server to use nip_hostname,
remove any insecure-skip-tls-verify keys under cluster entries, and add
tls-server-name: original_hostname under the same cluster.cluster mapping, then
serialize back with yaml.safe_dump (yaml.dump) to kubeconfig_content; add PyYAML
as a dependency and ensure you operate on all clusters/contexts rather than
relying on regex/idempotent string replacements.
In `@controller/deploy/microshift-bootc/config-svc/app.py`:
- Line 16: Fix the typo in the inline comment that reads "registrationfunctions"
by updating it to "registration functions" so the comment correctly reads
something like "Import route registration functions"; locate the comment near
the import block in app.py that currently contains "registrationfunctions" and
replace the single-word typo with the two-word phrase.
- Line 31: The default port for the app is set to 8080 (port =
int(os.environ.get('PORT', 8080))) which conflicts with the Containerfile and
run scripts that use 8880; update the default value to 8880 so that the PORT
fallback matches deployment and local dev (i.e., change the default in the PORT
getenv call from 8080 to 8880 in the app.py port assignment).
In `@controller/deploy/microshift-bootc/config-svc/system.py`:
- Around line 348-368: The json_to_yaml function currently writes a lone dash
line for list items that are dicts/lists, producing non-standard output; update
the list handling in json_to_yaml so when an item is a dict you render the dash
and the first key/value on the same line (e.g., "- key: value") and then append
remaining nested lines indented one extra level; locate the branch in
json_to_yaml where it detects isinstance(item, (dict, list)) and change it to
serialize the first line of the nested dict inline after the dash (using
yaml_value for simple scalars) and then call json_to_yaml for the rest of the
nested content with indent+1, ensuring subsequent lines are indented correctly
(reference json_to_yaml and yaml_value to implement this).
In
`@controller/deploy/microshift-bootc/config-svc/templates/password_required.html`:
- Around line 85-93: The code assumes result.messages is always an array and
calls result.messages.forEach which will throw if messages is missing or not an
array; update the response handling (the promise chain that processes
response.json and the block referencing result, result.messages,
messagesContainer, msg.type, msg.text) to defensively check that
Array.isArray(result.messages) (or that result.messages is truthy and iterable)
before iterating, and handle the else case by rendering a default error message
or skipping rendering so the UI doesn't crash.
In `@controller/deploy/microshift-bootc/Containerfile`:
- Around line 19-20: Add an explanatory comment above the RUN echo
"root:jumpstarter" | chpasswd line stating that the hardcoded default password
"jumpstarter" is intentional for initial boot, that the auth.py
is_default_password() check forces users to change it on first login, and note
the security implications so reviewers/users understand this is deliberate and
must not be used in production without changing; reference the
is_default_password() function in auth.py in the comment to tie behavior to
implementation.
In `@controller/deploy/microshift-bootc/Makefile`:
- Line 1: The .PHONY declaration at the top of the Makefile is missing the clean
target; update the .PHONY line (the line containing ".PHONY: help build
bootc-build ...") to include "clean" so the clean target is treated as phony and
not confused with a file of the same name—add "clean" to that list.
- Around line 76-92: The LVM cleanup in the build-image and build-iso targets
hardcodes the VG name "myvg1"; change it to use a Makefile variable (e.g.,
VG_NAME ?= myvg1) and reference that variable in the vgs/grep/vgremove commands
so different environments can override it (update the commands in the
build-image and build-iso targets that currently call vgs ... | grep myvg1 to
use $(VG_NAME)); ensure the variable has a sensible default and that both
targets are updated consistently.
In `@controller/deploy/microshift-bootc/run-microshift.sh`:
- Line 86: Extract the duplicate literal "jumpstarter.127-0-0-1.nip.io" into a
single read-only constant (e.g., HOSTNAME) alongside the other top-of-file
constants in run-microshift.sh, then replace the two inline definitions/usages
(the current local -r hostname="..." and the other occurrence) with references
to ${HOSTNAME}; ensure you remove the duplicated local -r hostname declaration
so both places use the single HOSTNAME constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5ed38e2-24eb-448b-8ff5-4e7a5ec967eb
📒 Files selected for processing (23)
.github/workflows/build-images.yamlcontroller/deploy/microshift-bootc/Containerfilecontroller/deploy/microshift-bootc/Makefilecontroller/deploy/microshift-bootc/README.mdcontroller/deploy/microshift-bootc/config-svc/README.mdcontroller/deploy/microshift-bootc/config-svc/__init__.pycontroller/deploy/microshift-bootc/config-svc/api.pycontroller/deploy/microshift-bootc/config-svc/app.pycontroller/deploy/microshift-bootc/config-svc/auth.pycontroller/deploy/microshift-bootc/config-svc/config-svc.servicecontroller/deploy/microshift-bootc/config-svc/pyproject.tomlcontroller/deploy/microshift-bootc/config-svc/routes.pycontroller/deploy/microshift-bootc/config-svc/system.pycontroller/deploy/microshift-bootc/config-svc/templates/index.htmlcontroller/deploy/microshift-bootc/config-svc/templates/password_required.htmlcontroller/deploy/microshift-bootc/config-svc/templates/styles.csscontroller/deploy/microshift-bootc/config-svc/update-banner.servicecontroller/deploy/microshift-bootc/config-svc/update-banner.shcontroller/deploy/microshift-bootc/config.tomlcontroller/deploy/microshift-bootc/kickstart.kscontroller/deploy/microshift-bootc/kustomization.yamlcontroller/deploy/microshift-bootc/output/.gitkeepcontroller/deploy/microshift-bootc/run-microshift.sh
.github/workflows/build-images.yaml
Outdated
Comment on lines
+46
to
+48
| - image_name: quay.io/jumpstarter-dev/microshift/bootc:latest | ||
| dockerfile: deploy/microshift-bootc/Containerfile | ||
| context: controller |
Contributor
There was a problem hiding this comment.
Incorrect image_name format will produce malformed image tags.
The image_name includes the full registry URL and tag (quay.io/jumpstarter-dev/microshift/bootc:latest), but other matrix entries use just org/repo format. The tag generation at lines 127-130 prepends ${{ env.REGISTRY }}/ and appends version tags, which would produce malformed references like:
quay.io/quay.io/jumpstarter-dev/microshift/bootc:latest:0.7.0
Additionally, the missing label field means PR label filtering (build-pr-images/<label>) won't work for this image.
Proposed fix
- - image_name: quay.io/jumpstarter-dev/microshift/bootc:latest
- dockerfile: deploy/microshift-bootc/Containerfile
- context: controller
+ - image_name: jumpstarter-dev/microshift-bootc
+ label: microshift-bootc
+ dockerfile: deploy/microshift-bootc/Containerfile
+ context: controller📝 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
| - image_name: quay.io/jumpstarter-dev/microshift/bootc:latest | |
| dockerfile: deploy/microshift-bootc/Containerfile | |
| context: controller | |
| - image_name: jumpstarter-dev/microshift-bootc | |
| label: microshift-bootc | |
| dockerfile: deploy/microshift-bootc/Containerfile | |
| context: controller |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-images.yaml around lines 46 - 48, The matrix entry
uses a full registry+tag in image_name which conflicts with the tag-generation
logic that prefixes ${{ env.REGISTRY }} and appends version tags; update the
image_name value (in the matrix entry currently set to
"quay.io/jumpstarter-dev/microshift/bootc:latest") to the org/repo format (e.g.,
"jumpstarter-dev/microshift/bootc") so generated tags become correct, and add a
matching label field (e.g., label: "bootc") so PR label filtering
(build-pr-images/<label>) works; adjust only the image_name and add label in
that matrix item to match the other entries and tagging logic.
Comment on lines
+103
to
+110
| try: | ||
| result = subprocess.run( | ||
| ['su', username, '-c', 'true'], | ||
| input=password.encode(), | ||
| capture_output=True, | ||
| timeout=5 | ||
| ) | ||
| return result.returncode == 0 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if python-pam is installed and available in the container
grep -r "python-pam\|pam" controller/deploy/microshift-bootc/ContainerfileRepository: jumpstarter-dev/jumpstarter
Length of output: 103
🏁 Script executed:
cat -n controller/deploy/microshift-bootc/config-svc/auth.py | head -120Repository: jumpstarter-dev/jumpstarter
Length of output: 4680
🏁 Script executed:
# Check if there are other Containerfiles or build configurations
find controller/deploy/microshift-bootc -name "Containerfile*" -o -name "*.dockerfile" -o -name "*.docker" | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 119
🏁 Script executed:
# Also check the deployment documentation or config for any conditional pam installation
grep -r "python-pam\|ImportError" controller/deploy/microshift-bootc --include="*.py" --include="*.md" --include="*.txt" --include="*.sh" -B 2 -A 2Repository: jumpstarter-dev/jumpstarter
Length of output: 1895
🏁 Script executed:
cat -n controller/deploy/microshift-bootc/Containerfile | grep -A 5 -B 5 "python-pam"Repository: jumpstarter-dev/jumpstarter
Length of output: 616
🏁 Script executed:
# Also check the full context around pip3 install to see if it's conditional
cat -n controller/deploy/microshift-bootc/Containerfile | grep -A 10 -B 10 "pip3 install"Repository: jumpstarter-dev/jumpstarter
Length of output: 937
Subprocess fallback authentication is unreliable and will fail in most scenarios.
The su command requires a TTY for password input and does not read passwords from stdin in typical configurations. While python-pam is unconditionally installed in the production Containerfile (guaranteeing the primary authentication path works), the fallback code at lines 103-110 will silently fail if pam import ever fails. Either remove the fallback entirely since pam is guaranteed, or implement a reliable alternative authentication method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/microshift-bootc/config-svc/auth.py` around lines 103 -
110, Remove the unreliable subprocess-based fallback that calls
subprocess.run(['su', username, '-c', 'true'], ...) for password checking; since
python-pam is guaranteed in the production image, delete that fallback block
from auth.py and make the module rely on the pam-based authentication path (or
raise a clear exception if pam import/initialization fails) so failures are not
silently ignored—ensure the authenticate function (the code path that currently
returns result.returncode == 0) either uses the pam API or surfaces an explicit
error when pam is unavailable.
Comment on lines
+4
to
+6
| After=network-online.target | ||
| Wants=network-online.target | ||
| Before=getty@.service systemd-user-sessions.service |
Contributor
There was a problem hiding this comment.
Don’t gate console logins on the web UI service.
Before=getty@.service systemd-user-sessions.service makes local login availability depend on config-svc reaching started state. Since this unit also waits on network-online.target, a slow or failed UI startup can unnecessarily delay recovery access. The banner updater already owns the “before getty” behavior, so this ordering should be removed from the web service unit.
Suggested change
After=network-online.target
Wants=network-online.target
-Before=getty@.service systemd-user-sessions.service📝 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
| After=network-online.target | |
| Wants=network-online.target | |
| Before=getty@.service systemd-user-sessions.service | |
| After=network-online.target | |
| Wants=network-online.target |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/microshift-bootc/config-svc/config-svc.service` around
lines 4 - 6, Remove the "Before=getty@.service systemd-user-sessions.service"
ordering from the config-svc unit so local console login isn't gated on the web
UI; update the unit file that contains the After=network-online.target and
Wants=network-online.target lines by deleting the Before=... entry (leave the
banner-updater unit to own the getty ordering instead).
Member
Author
There was a problem hiding this comment.
This is mostly for the banner.
Contributor
There was a problem hiding this comment.
@mangelajo, understood — the intent is to ensure the banner is shown before the first login prompt. However, since there's already a dedicated update-banner.service (oneshot) for exactly that purpose, the Before=getty@.service systemd-user-sessions.service ordering belongs there rather than in config-svc.service.
Keeping it in the web UI service means a slow or failed Flask startup (e.g. due to a network delay or app crash) would hold back the getty login prompt unnecessarily — which is the opposite of what you'd want during troubleshooting. The update-banner.service is the lighter, purpose-built unit that should own the "before getty" dependency.
Comment on lines
+33
to
+34
| [project.scripts] | ||
| jumpstarter-config-svc = "app_new:main" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify which module contains the main() function
echo "Checking for app_new.py:"
fd -t f "app_new.py" controller/deploy/microshift-bootc/config-svc/
echo "Checking for app.py:"
fd -t f "app.py" controller/deploy/microshift-bootc/config-svc/
echo "Checking main() function definition:"
rg -n "^def main\(\)" controller/deploy/microshift-bootc/config-svc/Repository: jumpstarter-dev/jumpstarter
Length of output: 274
Entry point references non-existent module app_new.
The console script entry point is configured as app_new:main, but app_new.py does not exist. The main() function is defined in app.py. This will cause an ImportError when running jumpstarter-config-svc.
🐛 Proposed fix
[project.scripts]
-jumpstarter-config-svc = "app_new:main"
+jumpstarter-config-svc = "app:main"📝 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
| [project.scripts] | |
| jumpstarter-config-svc = "app_new:main" | |
| [project.scripts] | |
| jumpstarter-config-svc = "app:main" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/microshift-bootc/config-svc/pyproject.toml` around lines 33
- 34, The console script entry jumpstarter-config-svc points to a non-existent
module app_new; update the entry point to reference the actual module that
defines main(). Either change the pyproject.toml [project.scripts] value from
"app_new:main" to "app:main" to match the existing app.py main() function, or
rename/create module app_new with a main() wrapper; ensure the symbol main() in
app.py (or the new module) is importable under the chosen module name.
Member
Author
There was a problem hiding this comment.
Ok this is weird, because it really works, can you double check?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me do a thorough check of the directory structure and all Python files.
🏁 Script executed:
#!/bin/bash
echo "=== All files in config-svc ==="
find controller/deploy/microshift-bootc/config-svc/ -type f | sort
echo ""
echo "=== Searching for 'app_new' anywhere ==="
rg -rn "app_new" controller/deploy/microshift-bootc/config-svc/
echo ""
echo "=== Checking app.py for main() and any app_new alias ==="
cat controller/deploy/microshift-bootc/config-svc/app.py
echo ""
echo "=== Checking __init__.py ==="
cat controller/deploy/microshift-bootc/config-svc/__init__.py 2>/dev/null || echo "No __init__.py at top level"
echo ""
echo "=== pyproject.toml scripts section ==="
grep -A5 "project.scripts" controller/deploy/microshift-bootc/config-svc/pyproject.toml
Length of output: 2611
Let me verify how the service is actually launched in the container to understand why it works.
🏁 Script executed:
#!/bin/bash
echo "=== Containerfile ==="
cat controller/deploy/microshift-bootc/Containerfile
echo ""
echo "=== Searching for how app.py / config-svc is invoked ==="
rg -n "app\.py\|config-svc\|app_new" controller/deploy/microshift-bootc/Containerfile
Length of output: 2118
@mangelajo, confirmed — you're right that it works! Here's why:
The Containerfile bypasses the pyproject.toml entry point entirely. Instead of installing the package via pip, it:
- Copies all
*.pyfiles directly to/usr/local/lib/config-svc/ - Creates a bash wrapper at
/usr/local/bin/config-svcthat executesexec /usr/bin/python3 app.py "$@"from that directory - The systemd service invokes that bash wrapper
So jumpstarter-config-svc = "app_new:main" in [project.scripts] is never called in this deployment. My original comment was technically correct that the pyproject.toml entry point is broken (since app_new.py doesn't exist), but it has no impact on the actual runtime behavior given the direct file-copy + bash wrapper deployment model.
You can either:
- Fix it to
"app:main"to keep thepyproject.tomlaccurate for anyone attempting a pip-based install, or - Leave it as-is if
pyproject.tomlis only used for metadata/tooling config and a pip-based install path is never intended.
Happy to close this comment if you'd like to leave it as-is!
🧠 Learnings used
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2026-02-04T18:12:55.555Z
Learning: Applies to python/packages/jumpstarter-driver-composite/pyproject.toml : The pyproject.toml for composite drivers should have a dependency on `jumpstarter-driver-composite`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2026-02-04T18:13:10.193Z
Learning: Applies to python/packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via `jumpstarter.drivers` entry point in their `pyproject.toml`
Member
Author
There was a problem hiding this comment.
ok I guess it works because of this:
Contributor
There was a problem hiding this comment.
@mangelajo, yes, exactly — that's the bash wrapper I identified:
exec /usr/bin/python3 app.py "$@"Since the deployment always goes through that wrapper rather than the pip-installed entry point, the app_new:main reference in pyproject.toml is dead code in practice. The comment is valid from a correctness standpoint (a pip install would fail), but it has zero impact on the actual container runtime. Up to you whether you want to fix it to "app:main" for hygiene, or leave it as-is.
🧠 Learnings used
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2026-02-04T18:12:55.555Z
Learning: Applies to python/packages/jumpstarter-driver-composite/pyproject.toml : The pyproject.toml for composite drivers should have a dependency on `jumpstarter-driver-composite`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2026-02-04T18:13:10.193Z
Learning: Applies to python/packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via `jumpstarter.drivers` entry point in their `pyproject.toml`
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2026-02-04T18:12:55.555Z
Learning: Applies to python/packages/jumpstarter-driver-*/**/{driver,driver_test}.py : Implement driver logic in the `driver.py` file and tests in `driver_test.py` file
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2026-02-04T18:13:10.193Z
Learning: Applies to python/packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py : Each driver package must include a `driver.py` file with driver implementation in `<package>/jumpstarter_driver_<name>/driver.py`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2026-02-04T18:13:10.193Z
Learning: Applies to python/packages/*/pyproject.toml : Package names use hyphens (e.g., `jumpstarter-driver-network`), while module names use underscores (e.g., `jumpstarter_driver_network`)
Comment on lines
+46
to
+53
| [tool.setuptools] | ||
| packages = ["config-svc"] | ||
|
|
||
| [tool.setuptools.package-data] | ||
| config-svc = [ | ||
| "templates/*.html", | ||
| "templates/*.css", | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual directory/file structure
echo "Directory structure:"
tree controller/deploy/microshift-bootc/config-svc/ 2>/dev/null || \
find controller/deploy/microshift-bootc/config-svc/ -type f -name "*.py" | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 474
🏁 Script executed:
cat -n controller/deploy/microshift-bootc/config-svc/pyproject.tomlRepository: jumpstarter-dev/jumpstarter
Length of output: 2456
🏁 Script executed:
# Search for imports of config-svc or config_svc in the codebase
rg -r "import.*config.?svc|from.*config.?svc" --type python -iRepository: jumpstarter-dev/jumpstarter
Length of output: 467
🏁 Script executed:
# Fix the rg pattern - search for imports of this package
rg "import.*config.*svc|from.*config.*svc" --type pythonRepository: jumpstarter-dev/jumpstarter
Length of output: 101
🏁 Script executed:
# Search for app_new references
rg "app_new" --type pythonRepository: jumpstarter-dev/jumpstarter
Length of output: 101
🏁 Script executed:
# Check if app_new.py exists
find controller/deploy/microshift-bootc/config-svc -name "app_new*"Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Search for imports without file type filter
rg "import.*config|from.*config" | grep -i svcRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Check Python files for imports
rg "^from|^import" controller/deploy/microshift-bootc/config-svc/*.py | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 2399
🏁 Script executed:
# Look for where this package is used from
rg "config.*svc|config_svc" --type-list | grep -i textRepository: jumpstarter-dev/jumpstarter
Length of output: 460
🏁 Script executed:
# Check if there's any build configuration that transforms names
cat controller/deploy/microshift-bootc/config-svc/__init__.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 142
🏁 Script executed:
# Look for any setup.py or other config files
find controller/deploy/microshift-bootc/config-svc -name "setup.py" -o -name "setup.cfg"Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Check how this package is actually deployed/used in the system
rg "config-svc|config_svc" controller/deploy/microshift-bootc/ | grep -v ".pyc" | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 2106
Fix package configuration - Python package names cannot contain hyphens.
The [tool.setuptools] section specifies packages = ["config-svc"], but Python package names must use underscores instead of hyphens. This breaks the package for any standard Python import.
Additionally, line 34 references a non-existent entry point: jumpstarter-config-svc = "app_new:main" should be "app:main" (the file is app.py, not app_new.py).
Rename the directory from config-svc to config_svc and update the configuration:
Changes needed
[tool.setuptools]
-packages = ["config-svc"]
+packages = ["config_svc"]
[tool.setuptools.package-data]
-config-svc = [
+config_svc = [
"templates/*.html",
"templates/*.css",
]Also fix the entry point on line 34:
[project.scripts]
-jumpstarter-config-svc = "app_new:main"
+jumpstarter-config-svc = "app:main"Then rename the directory:
mv controller/deploy/microshift-bootc/config-svc controller/deploy/microshift-bootc/config_svc
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/microshift-bootc/config-svc/pyproject.toml` around lines 46
- 53, The package name in pyproject.toml is invalid because it uses a hyphen:
update the [tool.setuptools] packages value from "config-svc" to "config_svc"
and adjust [tool.setuptools.package-data] key accordingly so setuptools matches
the actual package name; also fix the faulty entry point "jumpstarter-config-svc
= \"app_new:main\"" to reference the real module and callable ("app:main") and
rename the directory from config-svc to config_svc (e.g., mv
controller/deploy/microshift-bootc/config-svc
controller/deploy/microshift-bootc/config_svc) so package name, package-data and
the entry_point (app:main) are consistent.
Comment on lines
+38
to
+39
| output/qcow2/disk.qcow2: ## Build a bootable QCOW2 image from the bootc image | ||
| @echo "Building QCOW2 image from: $(BOOTC_IMG)"a |
Contributor
There was a problem hiding this comment.
Typo: Extra 'a' character in echo statement.
Line 39 has $(BOOTC_IMG)"a which will print an extra 'a' at the end of the message.
🐛 Proposed fix
output/qcow2/disk.qcow2: ## Build a bootable QCOW2 image from the bootc image
- `@echo` "Building QCOW2 image from: $(BOOTC_IMG)"a
+ `@echo` "Building QCOW2 image from: $(BOOTC_IMG)"📝 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
| output/qcow2/disk.qcow2: ## Build a bootable QCOW2 image from the bootc image | |
| @echo "Building QCOW2 image from: $(BOOTC_IMG)"a | |
| output/qcow2/disk.qcow2: ## Build a bootable QCOW2 image from the bootc image | |
| `@echo` "Building QCOW2 image from: $(BOOTC_IMG)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/microshift-bootc/Makefile` around lines 38 - 39, Remove the
stray character in the Makefile echo for the target output/qcow2/disk.qcow2: in
the recipe that echoes the BOOTC image (uses $(BOOTC_IMG)), delete the trailing
'a' so the message prints only the BOOTC image path; update the echo line in the
Makefile rule for output/qcow2/disk.qcow2 accordingly.
Comment on lines
+55
to
+63
| **Output example:** | ||
| ``` | ||
| MicroShift is running in a bootc container | ||
| Hostname: jumpstarter.10.0.2.2.nip.io | ||
| Container: jumpstarter-microshift-okd | ||
| LVM disk: /var/lib/microshift-okd/lvmdisk.image | ||
| VG name: myvg1 | ||
| Ports: HTTP:80, HTTPS:443, Config Service:8880 | ||
| ``` |
Contributor
There was a problem hiding this comment.
Mark the output/diagram fences with a language.
Both fenced blocks are missing a language, so markdown lint will keep warning on them. Use text for these examples.
Also applies to: 184-195
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 56-56: 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 `@controller/deploy/microshift-bootc/README.md` around lines 55 - 63, Update
the markdown fenced code blocks under the "Output example:" section (and the
other example block noted) to include a language tag so the linter stops
warning; specifically change the fences from ``` to ```text for the examples
shown in README.md (the "Output example:" block and the later example around
lines mentioned in the review) so both are marked as text-coded blocks.
mangelajo
force-pushed
the
microshift-bootc-image
branch
2 times, most recently
from
b40b57f to
ed676ed
Compare
mangelajo
force-pushed
the
microshift-bootc-image
branch
from
ed676ed to
92c65dd
Compare
Add Make targets to download CentOS Stream 10 boot ISO, embed the kickstart file using mkksiso, and upload the result to gofile.io. Supports both x86_64 and aarch64 via the ARCH variable. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
controller/deploy/microshift-bootc/Makefile (1)
47-48:⚠️ Potential issue | 🟡 MinorRemove the stray
ain the QCOW2 status message.Line 48 currently prints an extra character.
Suggested patch
output/qcow2/disk.qcow2: ## Build a bootable QCOW2 image from the bootc image - `@echo` "Building QCOW2 image from: $(BOOTC_IMG)"a + `@echo` "Building QCOW2 image from: $(BOOTC_IMG)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/Makefile` around lines 47 - 48, The status echo in the Makefile target output/qcow2/disk.qcow2 prints an extra stray character; update the echo statement in that target (which references $(BOOTC_IMG)) to remove the trailing "a" so the message reads correctly when building the QCOW2 image..github/workflows/build-images.yaml (2)
46-49:⚠️ Potential issue | 🟠 MajorAdd a
labelfor the new matrix entry to keep per-image PR builds working.Line 78 reads
matrix.label; without it, this entry can’t be selected viabuild-pr-images/<label>and gets skipped in label-targeted PR builds.Suggested patch
- image_name: jumpstarter-dev/microshift/bootc + label: microshift/bootc dockerfile: controller/deploy/microshift-bootc/Containerfile context: controller generate_installer: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-images.yaml around lines 46 - 49, The new matrix entry with image_name "jumpstarter-dev/microshift/bootc" is missing a matrix label, so PR builds targeted by label won't pick it up; add a `label:` key to that matrix item (e.g., label: microshift-bootc or bootc) so it can be referenced as `build-pr-images/<label>` in the workflow and selected via `matrix.label`.
130-134:⚠️ Potential issue | 🟠 MajorInstaller manifest uses the wrong operator tag on pull requests.
On PRs, pushed operator images are tagged
pr-<number>, but this step pinsIMGto${{ env.VERSION }}. That makesdist/install.yamlreference a tag that is typically not published for PR builds.Suggested patch
- name: Build operator installer manifest if: ${{ matrix.generate_installer && steps.check.outputs.skip != 'true' }} run: | - IMG="${{ env.QUAY_ORG }}/jumpstarter-operator:${{ env.VERSION }}" + if [[ "${{ github.event_name }}" == "pull_request" ]]; then + IMG_TAG="pr-${{ github.event.pull_request.number }}" + else + IMG_TAG="${{ env.VERSION }}" + fi + IMG="${{ env.QUAY_ORG }}/jumpstarter-operator:${IMG_TAG}" make -C controller/deploy/operator build-installer IMG="${IMG}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-images.yaml around lines 130 - 134, The installer manifest step hardcodes IMG to env.VERSION which is wrong for PR builds; update the step that defines IMG (used by make -C controller/deploy/operator build-installer) to choose the PR image tag for pull requests (e.g. pr-<number>) and fall back to env.VERSION otherwise—detect PRs via github.event_name == 'pull_request' and build the tag using github.event.number (so IMG becomes ${QUAY_ORG}/jumpstarter-operator:pr-<number> for PRs), then pass that IMG into the existing make invocation so dist/install.yaml references the actual pushed PR image tag.
🧹 Nitpick comments (1)
controller/deploy/microshift-bootc/Makefile (1)
1-1: Expand.PHONYto include all non-file targets (clean,bootc-reload-app).This prevents accidental file/target name collisions for operational targets.
Suggested patch
-.PHONY: help build bootc-build bootc-build-multi push bootc-push bootc-push-multi bootc-run bootc-stop bootc-sh bootc-rm build-image build-iso build-all build-all-multi push-all push-all-multi download-centos-iso build-ks-iso upload-ks-iso +.PHONY: help build bootc-build bootc-build-multi push bootc-push bootc-push-multi bootc-run bootc-stop bootc-sh bootc-rm bootc-reload-app build-image build-iso build-all build-all-multi push-all push-all-multi download-centos-iso build-ks-iso upload-ks-iso clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/deploy/microshift-bootc/Makefile` at line 1, The .PHONY line is missing non-file targets and should include the operational targets 'clean' and 'bootc-reload-app'; update the .PHONY declaration that currently lists targets like build/bootc-build/bootc-push etc. to also list clean and bootc-reload-app so Make treats them always as phony and avoids accidental file/target collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/deploy/microshift-bootc/Makefile`:
- Around line 88-99: The Makefile currently uses a dangerous global cleanup
command (sudo losetup -D) in the default image target and build-iso target;
replace that with targeted loop-device detachment that only affects devices
backing our artifacts (e.g., query losetup -j output/qcow2/disk.qcow2 or losetup
-j output/iso/disk.iso, parse the returned /dev/loopN and call sudo losetup -d
/dev/loopN for each) and keep the existing vg removal logic (or narrow vg
selection with --select vg_name=myvg1) so build-iso and the rule that creates
output/qcow2/disk.qcow2 detach only the specific loop devices for those files
instead of detaching all loop devices on the host.
---
Duplicate comments:
In @.github/workflows/build-images.yaml:
- Around line 46-49: The new matrix entry with image_name
"jumpstarter-dev/microshift/bootc" is missing a matrix label, so PR builds
targeted by label won't pick it up; add a `label:` key to that matrix item
(e.g., label: microshift-bootc or bootc) so it can be referenced as
`build-pr-images/<label>` in the workflow and selected via `matrix.label`.
- Around line 130-134: The installer manifest step hardcodes IMG to env.VERSION
which is wrong for PR builds; update the step that defines IMG (used by make -C
controller/deploy/operator build-installer) to choose the PR image tag for pull
requests (e.g. pr-<number>) and fall back to env.VERSION otherwise—detect PRs
via github.event_name == 'pull_request' and build the tag using
github.event.number (so IMG becomes ${QUAY_ORG}/jumpstarter-operator:pr-<number>
for PRs), then pass that IMG into the existing make invocation so
dist/install.yaml references the actual pushed PR image tag.
In `@controller/deploy/microshift-bootc/Makefile`:
- Around line 47-48: The status echo in the Makefile target
output/qcow2/disk.qcow2 prints an extra stray character; update the echo
statement in that target (which references $(BOOTC_IMG)) to remove the trailing
"a" so the message reads correctly when building the QCOW2 image.
---
Nitpick comments:
In `@controller/deploy/microshift-bootc/Makefile`:
- Line 1: The .PHONY line is missing non-file targets and should include the
operational targets 'clean' and 'bootc-reload-app'; update the .PHONY
declaration that currently lists targets like build/bootc-build/bootc-push etc.
to also list clean and bootc-reload-app so Make treats them always as phony and
avoids accidental file/target collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d823af4-6992-4800-afae-76bcb78598af
📒 Files selected for processing (2)
.github/workflows/build-images.yamlcontroller/deploy/microshift-bootc/Makefile
mangelajo
commented
Mar 13, 2026
| $(BOOTC_IMG) | ||
| @echo "QCOW2 image built successfully in ./output/" | ||
|
|
||
| output/iso/disk.iso: ## Build a bootable ISO image from the bootc image |
Member
Author
There was a problem hiding this comment.
This iso version is bigger (2GB) because it contains the bootc layers embedded in the iso, the other one (Cs10+kickstart) is 1GB
mangelajo
commented
Mar 13, 2026
| upload-ks-iso: ## Upload kickstart ISO to gofile.io and print download URL | ||
| @test -f "$(KS_ISO)" || { echo "Error: $(KS_ISO) not found. Run 'make build-ks-iso' first."; exit 1; } | ||
| @echo "Uploading $(KS_ISO) to gofile.io..." | ||
| @SERVER=$$(curl -s https://api.gofile.io/servers | jq -r '.data.servers[0].name') && \ |
Member
Author
There was a problem hiding this comment.
I am not sure how reputable gofile.io is, but I thought it's an easy way to share those isos for now, longer term we can stick them in github releases.
Container ImagesThe following container images have been built for this PR:
Images expire after 7 days. |
Member
Author
|
@kirkbrauer + all, it seems to work, even there are some / many improvements we can do, I suggest we just collaborate on main since I have very limited time for this now. |
Member
Author
|
This is an uploaded version of the iso image: https://gofile.io/d/wVVqfO |
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
New Features
Documentation
CI