Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds dry-run functionality to the trial command, allowing users to preview what actions would be taken without making actual changes to repositories. This is useful for understanding the impact of a trial workflow before execution.
Changes:
- Added a
--dry-runflag to the trial command that previews actions without executing them - Modified
ensureTrialRepositoryfunction to support dry-run mode with appropriate logging - Implemented early exit logic in
RunWorkflowTrialsto stop execution after dry-run preview
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cli/trial_command.go | Added DryRun field to TrialOptions struct, registered --dry-run flag, and implemented early exit logic when dry-run mode is enabled |
| pkg/cli/trial_repository.go | Updated ensureTrialRepository function signature to accept dryRun parameter and added conditional logic to preview instead of execute repository operations |
| pkg/cli/repo_error_messages_test.go | Updated test call to ensureTrialRepository with additional dryRun parameter |
Comments suppressed due to low confidence (1)
pkg/cli/trial_repository.go:56
- When both verbose and dry-run modes are enabled with forceDeleteHostRepo, two similar messages are printed: one at line 52 saying "Force deleting existing host repository" and another at line 56 saying "[DRY RUN] Would delete repository". This is redundant and could confuse users. Consider consolidating these into a single message or skipping the verbose message when in dry-run mode.
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Force deleting existing host repository: %s", repoSlug)))
}
if dryRun {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("[DRY RUN] Would delete repository: %s", repoSlug)))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if repoExists { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("[DRY RUN] Repository %s exists", repoSlug))) | ||
| } else { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("[DRY RUN] Repository %s does not exist (output: %s)", repoSlug, string(output)))) |
There was a problem hiding this comment.
The dry-run output exposes the error message when checking if a repository doesn't exist. This could potentially leak sensitive information or contain confusing technical details. Consider removing the output parameter from the message or limiting it to verbose mode only. A cleaner approach would be to simply state "[DRY RUN] Repository {repoSlug} does not exist" without including the raw error output.
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("[DRY RUN] Repository %s does not exist (output: %s)", repoSlug, string(output)))) | |
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("[DRY RUN] Repository %s does not exist", repoSlug))) |
| prefix := "" | ||
| if dryRun { | ||
| prefix = "[DRY RUN] " | ||
| } | ||
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("%sUsing existing host repository: https://github.com/%s", prefix, repoSlug))) |
There was a problem hiding this comment.
When reusing an existing repository in dry-run mode, the verbose information message (lines 69-74) is only shown when verbose flag is set, but the success message (line 80) is always shown with the dry-run prefix. For consistency in dry-run mode, consider showing the informational context (whether contents will be force-pushed) regardless of the verbose flag, similar to how the creation messages work (line 86 where dry-run implies showing the message).
See below for a potential fix:
prefix := ""
if dryRun {
prefix = "[DRY RUN] "
}
if verbose || dryRun {
if cloneRepoSlug != "" {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("%sReusing existing host repository: %s (contents will be force-pushed)", prefix, repoSlug)))
} else {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("%sReusing existing host repository: %s", prefix, repoSlug)))
}
}
| // In dry-run mode, stop here after showing what would be done | ||
| if opts.DryRun { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("[DRY RUN] Stopping here. No actual changes were made.")) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The new dry-run functionality lacks test coverage. Given that the codebase has comprehensive test coverage for similar flag-based functionality (as seen in the test files), consider adding unit tests to verify:
- The dry-run flag is correctly parsed and stored in TrialOptions
- The ensureTrialRepository function behaves correctly in dry-run mode (doesn't make actual API calls that modify state)
- The early exit logic at line 319-322 is triggered correctly in dry-run mode
- Dry-run messages are formatted consistently
This would help ensure the feature works as expected and prevent regressions.
Add documentation for the new --dry-run flag added to the trial command in PR #13804. This flag allows users to preview trial operations without executing workflows or creating repositories. Changes: - Updated CLI reference to include --dry-run flag and usage example - Added "Dry-Run Mode" section to TrialOps guide - Updated "Development Workflow" best practices to include dry-run step Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 participant
Summary
--dry-runflag for the trial command to preview actions without making changesensureTrialRepositoryfunction to support dry-run modeDetails
The changes enable users to run trial workflows in a preview mode, showing what actions would be taken without actually executing them. This is helpful for understanding the potential impact of a trial before committing to the full process.