Remove --create-pull-request and --push flags from add command#19016
Closed
Remove --create-pull-request and --push flags from add command#19016
add command#19016Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Remove PR creation behavior from add command
Remove --create-pull-request flag from Mar 1, 2026
add command
Contributor
|
@copilot split the interface types for add wizard and finish the cleanup |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Done in b7921d3. Split the types as follows:
|
Contributor
|
@copilot render the help of add and add-wizard |
Contributor
|
@copilot remove the --push option of add command. The add command should not interact with git. Only the add-wizard does git operations. |
Contributor
Author
Both
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Done in 57f9a23. Removed Updated help:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Remove --create-pull-request flag from
Remove --create-pull-request and --push flags from Mar 1, 2026
add commandadd command
Contributor
|
I'd expect the flags to still be on "add". I mean why not, people like to automate these things? It's the "do things without asking" we want to convert to "do things either by asking (add-wizard) or opt in flag (add)" |
Contributor
|
/scout research how add/install commands behaves in the top 5 most popular package/library frameworks and compare the behaviors of how libraries are added. |
Contributor
🔍 Scout Research ReportTriggered by Executive SummaryAll top package managers follow a consistent pattern: Detailed Findings by Package Manager1. npm —
|
| Behavior | Details |
|---|---|
| Command | npm install (package) or npm add (package) (official alias) |
| Manifest | Adds entry to package.json dependencies or devDependencies |
| Lockfile | Updates package-lock.json |
| Git ops | ❌ None — developer commits manually |
| PR creation | ❌ None |
Key note: npm install has many [command aliases]((docs.npmjs.com/redacted) including add, i, in, ins, etc. All are treated identically.
2. Yarn — yarn add (package)
| Behavior | Details |
|---|---|
| Command | yarn add (package) |
| Manifest | Adds entry to package.json immediately (no --save flag needed) |
| Lockfile | Updates yarn.lock |
| Git ops | ❌ None — developer commits manually |
| PR creation | ❌ None |
Key differentiator from npm: Yarn chose add over install as the primary verb for adding a new package, while yarn (no args) installs all existing dependencies. The naming makes the intent explicit.
3. pip — pip install (package)
| Behavior | Details |
|---|---|
| Command | pip install (package) |
| Manifest | Installs to site-packages/; does not auto-update requirements.txt (manual pip freeze needed) or pyproject.toml |
| Lockfile | None native (tools like Poetry/uv add this) |
| Git ops | ❌ None |
| PR creation | ❌ None |
Note: Modern tools like uv add and poetry add do update pyproject.toml/lockfile, but still stop there — no git or PR operations.
4. Cargo (Rust) — cargo add (crate)
| Behavior | Details |
|---|---|
| Command | cargo add (crate) |
| Manifest | Adds entry under [dependencies] in Cargo.toml |
| Lockfile | Updates Cargo.lock on next build |
| Git ops | ❌ None — Cargo does not touch git at all after adding a dependency |
| PR creation | ❌ None |
cargo add is explicitly a manifest command (listed under "Manifest Commands" in the Cargo book). cargo install is a separate concept that installs a binary to $GOPATH/bin, not a project dependency.
5. Go modules — go get (module)
| Behavior | Details |
|---|---|
| Command | go get (module) |
| Manifest | Adds require directive to go.mod |
| Lockfile | Updates go.sum (checksum database) |
| Git ops | ❌ None — go get downloads to module cache only |
| PR creation | ❌ None |
Note: Since Go 1.16, go get is dedicated to dependency management (updating go.mod), while go install is for building/installing binaries. A clean separation of concerns.
6. dotnet/NuGet — dotnet add package (name)
| Behavior | Details |
|---|---|
| Command | dotnet add package (name) (or dotnet package add in .NET 10+) |
| Manifest | Adds (PackageReference) element to .csproj file; runs dotnet restore automatically |
| Lockfile | Restores packages to ~/.nuget/packages cache |
| Git ops | ❌ None |
| PR creation | ❌ None |
Universal Pattern
Developer runs: add / install (package)
Package manager: 1. Resolves version
2. Downloads package
3. Updates manifest (package.json / Cargo.toml / go.mod / .csproj)
4. Updates lockfile (if applicable)
5. STOPS ← no git, no commit, no push, no PR
Developer decides: when and how to commit, push, and open a PR
Recommendations
- The PR is well-aligned with industry norms. Removing
--create-pull-request/--pushfromaddfollows the universal convention that packageadd/installcommands are local manifest operations only. - Naming is consistent with Yarn's precedent. Yarn popularized the
addverb specifically for the "add a new dependency" action vs.install(install all existing). Both have clear, narrowly scoped behavior. - Separation of concerns is the right approach. Git operations (branch, commit, push) and PR creation belong to the wizard/orchestration layer — exactly what
add-wizard(AddResolvedWorkflowsWithPR) now encapsulates. - No package manager auto-commits. If the
addcommand were to auto-commit, it would be unique among all major package managers in a way that would surprise engineers onboarding to the tool.
Key Sources
- [npm install docs (aliases: add, i, ...)]((docs.npmjs.com/redacted)
- [Yarn vs npm command cheat sheet]((shift.infinite.red/redacted)
- [pip install documentation]((pip.pypa.io/redacted)
- [The Cargo Book — cargo add]((doc.rustlang.org/redacted)
- [Go modules — managing dependencies]((go.dev/redacted)
- dotnet add package — NuGet CLI
References: §22546670994
🔭 Intelligence gathered by Scout
Contributor
|
Hey One issue needs to be addressed before this is ready:
If you'd like a hand, you can assign this prompt to your coding agent:
|
Contributor
|
@copilot review docs to determine if any sample relies on |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the add command to behave more like a standard “package add/install” command by removing git/PR side effects from the non-interactive path and moving PR creation responsibilities into the interactive add wizard flow.
Changes:
- Removed
--create-pull-request/--prand--pushflags and the associated PR/push execution paths fromadd. - Introduced a wizard-specific
AddWizardResultandAddResolvedWorkflowsWithPRhelper to encapsulate branch/commit/push/PR creation for interactive flows. - Updated interactive orchestration to consume the new wizard result type; updated unit tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/add_command.go | Removes PR/push flags and deletes PR/push behavior from the non-interactive add execution path. |
| pkg/cli/add_workflow_pr.go | Adds wizard-specific result type and a PR-creating workflow-add entrypoint for interactive usage. |
| pkg/cli/add_interactive_git.go | Switches interactive apply flow to call the new PR-specific add function directly. |
| pkg/cli/add_interactive_orchestrator.go | Updates stored add result type to the new wizard result. |
| pkg/cli/add_command_test.go | Removes assertions for deleted flags/fields and simplifies result tests. |
| pkg/cli/add_workflow_pr_test.go | Adds a new test for the wizard result type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/cli/add_workflow_pr_test.go
Outdated
Comment on lines
202
to
254
| func TestAddWizardResult(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| prNumber int | ||
| prURL string | ||
| hasWorkflowDispatch bool | ||
| }{ | ||
| { | ||
| name: "default values", | ||
| prNumber: 0, | ||
| prURL: "", | ||
| hasWorkflowDispatch: false, | ||
| }, | ||
| { | ||
| name: "with PR number", | ||
| prNumber: 123, | ||
| prURL: "", | ||
| hasWorkflowDispatch: false, | ||
| }, | ||
| { | ||
| name: "with PR URL", | ||
| prNumber: 0, | ||
| prURL: "https://github.com/owner/repo/pull/123", | ||
| hasWorkflowDispatch: false, | ||
| }, | ||
| { | ||
| name: "with workflow dispatch", | ||
| prNumber: 0, | ||
| prURL: "", | ||
| hasWorkflowDispatch: true, | ||
| }, | ||
| { | ||
| name: "all fields set", | ||
| prNumber: 456, | ||
| prURL: "https://github.com/owner/repo/pull/456", | ||
| hasWorkflowDispatch: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := &AddWizardResult{ | ||
| PRNumber: tt.prNumber, | ||
| PRURL: tt.prURL, | ||
| HasWorkflowDispatch: tt.hasWorkflowDispatch, | ||
| } | ||
|
|
||
| assert.Equal(t, tt.prNumber, result.PRNumber, "PRNumber should match") | ||
| assert.Equal(t, tt.prURL, result.PRURL, "PRURL should match") | ||
| assert.Equal(t, tt.hasWorkflowDispatch, result.HasWorkflowDispatch, "HasWorkflowDispatch should match") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestAddWizardResult only verifies that struct fields round-trip after assignment, which doesn’t exercise any behavior and won’t catch regressions. Consider replacing this with targeted tests for AddResolvedWorkflowsWithPR (e.g., deterministic failure paths like “gh not available” via t.Setenv("PATH", ""), and “not a git repo” via os.Chdir(t.TempDir())) so the newly introduced PR-wizard logic has meaningful coverage.
Suggested change
| func TestAddWizardResult(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| prNumber int | |
| prURL string | |
| hasWorkflowDispatch bool | |
| }{ | |
| { | |
| name: "default values", | |
| prNumber: 0, | |
| prURL: "", | |
| hasWorkflowDispatch: false, | |
| }, | |
| { | |
| name: "with PR number", | |
| prNumber: 123, | |
| prURL: "", | |
| hasWorkflowDispatch: false, | |
| }, | |
| { | |
| name: "with PR URL", | |
| prNumber: 0, | |
| prURL: "https://github.com/owner/repo/pull/123", | |
| hasWorkflowDispatch: false, | |
| }, | |
| { | |
| name: "with workflow dispatch", | |
| prNumber: 0, | |
| prURL: "", | |
| hasWorkflowDispatch: true, | |
| }, | |
| { | |
| name: "all fields set", | |
| prNumber: 456, | |
| prURL: "https://github.com/owner/repo/pull/456", | |
| hasWorkflowDispatch: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| result := &AddWizardResult{ | |
| PRNumber: tt.prNumber, | |
| PRURL: tt.prURL, | |
| HasWorkflowDispatch: tt.hasWorkflowDispatch, | |
| } | |
| assert.Equal(t, tt.prNumber, result.PRNumber, "PRNumber should match") | |
| assert.Equal(t, tt.prURL, result.PRURL, "PRURL should match") | |
| assert.Equal(t, tt.hasWorkflowDispatch, result.HasWorkflowDispatch, "HasWorkflowDispatch should match") | |
| }) | |
| } | |
| } |
pkg/cli/add_workflow_pr.go
Outdated
| // AddResolvedWorkflowsWithPR adds workflows using pre-resolved workflow data and creates a pull request. | ||
| // This is the wizard-specific variant of AddResolvedWorkflows used by the interactive add flow. | ||
| func AddResolvedWorkflowsWithPR(workflowStrings []string, resolved *ResolvedWorkflows, opts AddOptions) (*AddWizardResult, error) { | ||
| addWorkflowPRLog.Printf("Adding workflows with PR: count=%d", len(workflowStrings)) |
There was a problem hiding this comment.
In AddResolvedWorkflowsWithPR, the log line reports count based on len(workflowStrings), which can be misleading when a single spec expands to multiple resolved workflows (e.g., local wildcard). Consider logging len(resolved.Workflows) (and/or logging both values) so the count reflects the actual number of workflows being added.
Suggested change
| addWorkflowPRLog.Printf("Adding workflows with PR: count=%d", len(workflowStrings)) | |
| addWorkflowPRLog.Printf( | |
| "Adding workflows with PR: specs=%d, resolved_workflows=%d", | |
| len(workflowStrings), | |
| len(resolved.Workflows), | |
| ) |
… flags Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Found and fixed in 59df85a. Two doc files referenced the removed flags:
The |
Contributor
|
@copilot apply changes based on the comments in this thread |
…t with failure-path tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Done in d083d89. Applied both reviewer suggestions:
|
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
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.
--create-pull-request/--prflags andCreatePRfromAddOptions/AddWorkflowsResultAddWizardResultandAddResolvedWorkflowsWithPRfor wizard-only PR concerns--push/Pushflag fromaddcommand — git operations belong to the add-wizard onlyadd --create-pull-request,--pr, and--pushfromdocs/src/content/docs/setup/cli.mdanddocs/src/content/docs/guides/packaging-imports.mdAddResolvedWorkflowsWithPRto report both spec count (specs=) and resolved workflow count (resolved_workflows=)TestAddWizardResultstruct round-trip test withTestAddResolvedWorkflowsWithPR_GHNotAvailableandTestAddResolvedWorkflowsWithPR_NotGitRepoto cover real failure pathsOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.