Skip to content

Tweak BuilderNet recipe defaults#389

Closed
canercidam wants to merge 3 commits intofryd/improve-buildernet-mkosi-readmefrom
caner/change-mkosi-default-branch
Closed

Tweak BuilderNet recipe defaults#389
canercidam wants to merge 3 commits intofryd/improve-buildernet-mkosi-readmefrom
caner/change-mkosi-default-branch

Conversation

@canercidam
Copy link
Member

No description provided.

@claude
Copy link
Contributor

claude bot commented Mar 2, 2026

Claude finished @canercidam's task in 34s —— View job


Review

  • Gather context and review changes
  • Post review feedback

The change switches the default branch for the flashbots-images clone from main to fryd/mkosi-playground.

Potential concern:

Defaulting to a feature branch (fryd/mkosi-playground) rather than a stable branch like main is fragile — if that branch is deleted, renamed, or force-pushed, this script will break for anyone running it fresh. If this branch hasn't been merged to main yet, consider noting in the PR description or a code comment that this default should be reverted once the upstream branch is merged.

Additionally, the script has a pre-existing gap: if .flashbots-images already exists (cloned from a different branch), the clone is skipped entirely and the old branch content is used silently. This isn't introduced by this PR, but it becomes more relevant now that the default is a feature branch — users who previously ran the script against main won't pick up the branch change without manually deleting the directory.

No blocking issues — the change is correct for what it intends to do.

@canercidam canercidam changed the title Change the default branch for the mkosi image Tweak BuilderNet recipe defaults Mar 2, 2026
@claude
Copy link
Contributor

claude bot commented Mar 2, 2026

Claude finished @canercidam's task in 46s —— View job


Review

  • Gather context and review changes
  • Post review feedback

Two commits change the default flashbots-images branch from main to fryd/mkosi-playground and update the README Quick Start to reference build.sh and drop --detached.

Feature branch as default (build.sh:10): Defaulting to fryd/mkosi-playground is fragile — if that branch is deleted, renamed, or force-pushed, build.sh will break for fresh users. Consider adding a comment noting this should be reverted once the upstream branch merges to main.

Stale clone not updated (build.sh:12-15): The clone is skipped when .flashbots-images already exists. Users who previously ran the script against main won't pick up the branch change without manually deleting the directory. This is pre-existing, but more impactful now that the default is a feature branch. Consider adding a branch check or at least a warning.

--detached removed from README (README.md:39): The Quick Start previously had --detached; now it doesn't. This changes user experience (foreground vs background). If intentional, fine — just noting it's a behavioral change included in what looks like a branch-default PR.

No blocking issues.

@claude
Copy link
Contributor

claude bot commented Mar 2, 2026

Claude finished @canercidam's task in 59s —— View job


Review

  • Gather context and review changes
  • Post review feedback

Three commits: default branch change in build.sh, README Quick Start updates, and a small warning comment.

No new issues beyond what was flagged in prior reviews. The key concern remains the feature branch default (build.sh:10) — previous inline comment about adding a TODO stands. The README improvements are straightforward and reasonable.

One minor nit on the latest commit: the (ignore if you're reading the generated README) comment at line 29 assumes the reader knows what "generated README" means. Since builder-playground generate copies this file, it would be clearer as something like # (skip if you already ran 'generate').

No blocking issues.

Copy link
Contributor

@fkondej fkondej left a comment

Choose a reason for hiding this comment

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

one comment, rest LGTM.

# - alternatively you can set this in playground.yaml (see below)
# 3. Build a VM image or point to it
# Build
./scripts/build.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

The BuilderNet VM should be built according to flashbots-images instructions. Those might change.
I have added builder.sh as an example, and it shouldnt be used as part of regular workflow.

@canercidam canercidam closed this Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants