--attach stack 7/8: Add the flag to gh pr create and gh pr edit - #14183
Conversation
BagToad
changed the title
--attach stack 7/8: Add the flag to gh pr create and gh pr edit
There was a problem hiding this comment.
Pull request overview
Adds --attach support to gh pr create and gh pr edit.
Changes:
- Uploads images/videos and rewrites or appends body references.
- Handles conflicts, partial failures, permissions, and host-specific authentication.
- Adds comprehensive create/edit tests and configurable repository mocks.
Show a summary per file
| File | Description |
|---|---|
pkg/httpmock/legacy.go |
Extends repository stubs with IDs and permissions. |
pkg/cmd/pr/edit/edit.go |
Integrates attachments into PR editing. |
pkg/cmd/pr/edit/edit_test.go |
Tests edit attachment behavior and failures. |
pkg/cmd/pr/create/create.go |
Integrates attachments into PR creation. |
pkg/cmd/pr/create/create_test.go |
Tests create attachment flows and conflicts. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
7c5b71f to
87ba0ca
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
87ba0ca to
0a631eb
Compare
babakks
left a comment
babakks
left a comment
There was a problem hiding this comment.
Thanks for another clean PR in the stack, @BagToad! 🚀
This wires --attach into gh pr create and gh pr edit and it reads well. The uploader is built before any prompting, the partial-upload path is handled sensibly (the PR is still created/updated with what succeeded, the URL is printed, and the command exits non-zero), and the test coverage is solid.
I left a handful of inline notes, all non-blocking:
- A couple of help-text nitpicks: an extra example showing repeated
--attachwith no alt text, and a line documenting the partial-upload behavior on both commands. - A test-simplification idea: for the "must never prompt/survey" cases, we could leave the prompter and surveyor stubs
niland rely on the resulting panic, instead of hand-writing error-returning stubs. - One design question I keep coming back to across the stack:
FromFlagValuestouches the filesystem during the pre-run flag step to confirm the asset files exist. A flag parsing/validation phase ideally shouldn't be doing existence checks. No concrete proposal yet, just flagging it for a possible follow-up.
Nothing here needs to block the merge. Nice work! 🎉
| opts.Assets, err = attachments.FromFlagValues(cmd) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
question: I know it's been like this since the previous PR for the issue comment and pr comment additions, so I'm just raising it to see if we can make a slight improvement here. This FromFlagValues call consults the filesystem to verify the asset files exist. Ideally a pre-run flag parsing/validation step shouldn't get involved in such checks. I know we've probably already broken this in some commands, but it's still worth calling out.
I have no thought-through idea on how to do it at the moment, so please let me know if you see a nice way. My main concern is that we'd end up wanting two validation steps: one for flag-related data (e.g. parsing the alt text) and one for the actual run (e.g. checking the files exist). 🤔
There was a problem hiding this comment.
I'm not sure on this one; bit on the fence.
I guess my take is that the flag validation step always happens before we mutate, and validating this stuff early helps us in this case because NOT uploading when something is wrong is so vital - we're protecting against unattached assets. This sort of forces it to be validated before we mutate.
That's not the only way to guarantee this of course. And maybe it's not even the best way to guarantee this.
Part of the pull request stack tracked in #14186.
Description
This is the seventh layer of the stack. It adds
--attachtogh pr createandgh pr edit, one commit each.On create, the body can come from
--body, from--body-file, from standard input, from a text editor, or from--fill, which builds the body from the commits. All of them work with an attachment, and a reference written in any of them is rewritten in place.On edit, using only
--attachwith no body flag keeps the existing body and adds the attachment below it. If the existing body already references the file, the reference is rewritten in place and nothing is appended.Two combinations are refused on create.
--attachwith--webcannot work.--attachwith--dry-runis refused because a dry run should not upload anything, and an upload cannot be undone.How did you test this change?
Both commands were exercised by hand against a private test repository. That covered a pull request created with an image, one created with a video, one created from a body file whose reference was rewritten, and one created with
--fillwhere the generated body was kept and the asset appended below it rather than replacing it.On edit it covered appending to an existing body, and editing where the reference already in the body was rewritten in place. Both refused flag combinations were checked.
A failed rewrite was also exercised. A body that embeds a video through a reference style definition cannot be rewritten, and in that case the command exited non zero, the existing body was left exactly as it was, and nothing was printed to standard output.
Key points
The failure case is the one worth reading closely. When the markdown cannot be rewritten, the existing body survives untouched and no URL is printed, so the run does not read as a success to a script.
Notes for reviewers
The two commits are independent and can be read in either order. Editing is the more interesting of the two, because it has an existing body to preserve and creating does not.
Authorship and follow-up
Who wrote this:
Who answers review comments: