You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Bug (crash at startup):generateCmd.Flags().AddFlagSet(recipe.Flags()) in main.go:610 adds flags from all recipes to a single command. Recipes share flag names (e.g. --block-time in both L1 and OpStack, all L1 flags duplicated via BuilderNet). pflag panics on flag redefinition, so this will crash the binary when the generate command is initialized. (inline comment)
Path resolution inconsistency:filepath.Abs in recipe_yaml.go resolves host_path relative to CWD, not the recipe directory. Other relative paths in the same file use recipeDir. This will break when the user's CWD differs from the recipe file's location. (inline comment)
Minor — protocol not updated on port override:WithPort in manifest.go updates the port number but returns before updating the protocol. Not a problem for current callers (all use the default TCP), but makes the API incorrect. (inline comment)
The test_tx.go changes (timeout/retries) and local_runner_lifecycle.go (setup commands) look correct.
Re-review of PR #379 (after feedback commit 76970b1)
Read updated diff and changed files
Review each changed file for issues
Post review summary
Summary
The feedback commit addresses two of the three issues from my previous review:
WithPort protocol update — Fixed. p.Protocol = protocol is now set when overriding the port number (manifest.go:530).
host_path resolution — Fixed. Relative paths now resolve via filepath.Join(recipeDir, ...) instead of bare filepath.Abs (recipe_yaml.go:476-480, 657-661).
generateCmd flag duplication — Acknowledged as deferred (main.go:612). This will panic at startup due to conflicting --block-time flag types across recipes (Duration in L1, Uint64 in OpStack). Since it's marked as a known issue, no further comment.
No new issues found in the updated diff. The rest of the changes (setup commands, test retries/timeout, skip-setup flag) look correct.
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
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.
generatecmd to support conditional recipe generation--retriesflag totestcommand and improve timeout flag usagesetupfield to custom YAML recipes to setup the environment first before running the playground - arrives with--skip-setupoptionhost_paths to absolute paths to run local binaries from custom recipesThese are improvements made and discovered during the experimentation with op-rbuilder repo, for running locally and in CI.