fix: gh pr create -w ignore template flag
#9863
Merged
Conversation
This file contains 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
cliAutomation
added
the
external
jtmcg
requested changes
Nov 5, 2024
There was a problem hiding this comment.
Hey @nilvng, looks good so far. I've tested your code and it's working as expected.
That said, what happens if you pass in an incorrect template string? It seems that in interactive mode we have validation to sort out whether or not the template you've passed exists:
$ gh pr create --template=nonexistent_pr_template.md
Creating pull request for test-branch into main in jtmcg/gh-pr-create-web-with-template
? Title (required) test
X operation failed. To restore: gh pr create --recover /var/folders/9k/1_l7g_sn6bx2nxkfsjdr18vm0000gn/T/gh1585674906.json
template "nonexistent_pr_template.md" not foundI think we should include this kind of validation in our approach for --web mode here as well. Specifically, if a template does not exist, we should fail in the terminal before opening the browser.
I think the validation in the above command is coming from templateManager.Select(). It seems that the manager is fetching the templates from the API, then checking that the template string passed in matches one of the templates that has been fetched. I think we should implement similar behavior, here.
|
@jtmcg I just realised that #7804 is pretty much a duplicate of #9325. There's a bunch of context in there about how the Web flow works a bit different than the API. Specifically, you'll see in my A/C that:
So with that in mind, I don't think we want to do validation here. It's a gray area but I think the advantage of allowing people to get the same results as they can in the browser outweighs the earlier validation. Let me know your thoughts! |
|
hey @jtmcg and @williammartin thank you for taking time to provide the feedbacks! I have only been exposed to such a small part of the repo - def missed out on the I'm open to either of your suggestions - if theres a way that could help capturing the decision behind non-validated template. Let me know what you think! |
|
Ah, that context is super helpful @williammartin. Thanks! As such, @nilvng, I think we can ignore the ask for template validation in this case 🙌 I am confused about this commit, though. I don't remember seeing these changes when I reviewed yesterday (though that was probably my fault given they were added before my review), but those changes seem out of scope for both #7804 & #9325. Where they meant to address another issue? If so, let's split this commit out into a different PR and keep this one scoped to addressing the |
|
Thanks for the contribution, @nilvng |
|
its my pleasure ❤️ |
tmeijn
pushed a commit
to tmeijn/dotfiles
that referenced
this pull request
Nov 21, 2024
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.59.0` -> `v2.62.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.62.0`](https://github.com/cli/cli/releases/tag/v2.62.0): GitHub CLI 2.62.0 [Compare Source](cli/cli@v2.61.0...v2.62.0) #### What's Changed - Update monotonic verification logic and testing by [@​malancas](https://github.com/malancas) in cli/cli#9856 - Check extension for latest version when executed by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9866 - Shorten extension release checking from 3s to 1s by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9914 - Mention GitHub CLI team on discussion issues by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9920 **Full Changelog**: cli/cli@v2.61.0...v2.62.0 #### Security - A security vulnerability has been identified in GitHub CLI that could allow remote code execution (RCE) when users connect to a malicious Codespace SSH server and use the `gh codespace ssh` or `gh codespace logs` commands. For more information, see GHSA-p2h2-3vg9-4p87 #### GitHub CLI notifies users about latest extension upgrades Similar to the notification of latest `gh` releases, the `v2.62.0` version of GitHub CLI will notify users about latest extension upgrades when the extension is used: ```shell $ gh ado2gh ... A new release of ado2gh is available: 1.7.0 → 1.8.0 To upgrade, run: gh extension upgrade ado2gh --force https://github.com/github/gh-ado2gh ``` ##### Why does this matter? This removes a common pain point of extension authors as they have had to reverse engineer and implement a similar mechanism within their extensions directly. With this quality of life improvement, there are 2 big benefits: 1. Extension authors will hopefully see increased adoption of newer releases while having lower bar to maintaining their extensions. 2. GitHub CLI users will have greater awareness of new features, bug fixes, and security fixes to the extensions used. ##### What do you need to do? Extension authors should review their extensions and consider removing any custom logic previously implemented to notify users of new releases. ### [`v2.61.0`](https://github.com/cli/cli/releases/tag/v2.61.0): GitHub CLI 2.61.0 [Compare Source](cli/cli@v2.60.1...v2.61.0) #### Ensure users understand consequences before making repository visibility changes In `v2.61.0`, `gh repo edit` command has been enhanced to inform users about [consequences of changing visibility](https://gh.io/setting-repository-visibility) and ensure users are intentional before making irreversible changes: 1. Interactive `gh repo edit` visibility change requires confirmation when changing from `public`, `private`, or `internal` 2. Non-interactive `gh repo edit --visibility` change requires new `--accept-visibility-change-consequences` flag to confirm 3. New content to inform users of consequences - Incorporate [GitHub Docs content](https://gh.io/setting-repository-visibility) into help usage and interactive `gh repo edit` experience - Expanded help usage to call out most concerning consequences - Display repository star and watcher counts to understand impact before confirming #### What's Changed - Add acceptance test for `project` command by [@​jtmcg](https://github.com/jtmcg) in cli/cli#9816 - Add comprehensive testscript for `gh ruleset` by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9815 - Add comprehensive testscript for gh ext commandset by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9810 - Require visibility confirmation in `gh repo edit` by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9845 - Clean up skipped online tests for `gh attestation verify` by [@​malancas](https://github.com/malancas) in cli/cli#9838 - `gh attestation verify` should only verify provenance attestations by default by [@​malancas](https://github.com/malancas) in cli/cli#9825 - Set `dnf5` commands as default by [@​its-miroma](https://github.com/its-miroma) in cli/cli#9844 - Fix verbiage for deleting workflow runs by [@​akx](https://github.com/akx) in cli/cli#9876 - Bump github.com/creack/pty from 1.1.23 to 1.1.24 by [@​dependabot](https://github.com/dependabot) in cli/cli#9862 - `gh attestation verify` policy enforcement refactor by [@​malancas](https://github.com/malancas) in cli/cli#9848 - Simplify Sigstore verification result handling in `gh attestation verify` by [@​malancas](https://github.com/malancas) in cli/cli#9877 - Print empty array for `gh cache list` when `--json` is provided by [@​williammartin](https://github.com/williammartin) in cli/cli#9883 - Bump actions/attest-build-provenance from 1.4.3 to 1.4.4 by [@​dependabot](https://github.com/dependabot) in cli/cli#9884 - Create the automatic key when specified with -i by [@​cmbrose](https://github.com/cmbrose) in cli/cli#9881 - fix: `gh pr create -w` ignore template flag by [@​nilvng](https://github.com/nilvng) in cli/cli#9863 #### New Contributors - [@​akx](https://github.com/akx) made their first contribution in cli/cli#9876 - [@​nilvng](https://github.com/nilvng) made their first contribution in cli/cli#9863 **Full Changelog**: cli/cli@v2.60.1...v2.61.0 ### [`v2.60.1`](https://github.com/cli/cli/releases/tag/v2.60.1): GitHub CLI 2.60.1 [Compare Source](cli/cli@v2.60.0...v2.60.1) This is a small patch release to fix installing `gh` via `go install` which was broken with v2.60.0. #### What's Changed - Update testscript to use hard fork by [@​williammartin](https://github.com/williammartin) in cli/cli#9821 **Full Changelog**: cli/cli@v2.60.0...v2.60.1 ### [`v2.60.0`](https://github.com/cli/cli/releases/tag/v2.60.0): GitHub CLI 2.60.0 [Compare Source](cli/cli@v2.59.0...v2.60.0) #### What's Changed - Add ArchivedAt field by [@​tsukasaI](https://github.com/tsukasaI) in cli/cli#9790 - Include startedAt, completedAt in run steps data by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9774 - Adjust environment help for host and tokens by [@​williammartin](https://github.com/williammartin) in cli/cli#9809 - Add handling of empty titles for Issues and MRs by [@​jtmcg](https://github.com/jtmcg) in cli/cli#9701 - `LiveSigstoreVerifier.Verify` should error if no attestations are present by [@​phillmv](https://github.com/phillmv) in cli/cli#9742 - `gh at verify` retries fetching attestations if it receives a 5xx by [@​phillmv](https://github.com/phillmv) in cli/cli#9797 - Prevent local extension installations with invalid names and conflicts with core commands and other extensions by [@​BagToad](https://github.com/BagToad) in cli/cli#9794 - Rewrite a sentence in CONTRIBUTING.md by [@​muzimuzhi](https://github.com/muzimuzhi) in cli/cli#9772 - Use new GitHub preview terms in `working-with-us.md` by [@​BagToad](https://github.com/BagToad) in cli/cli#9800 - Use new GitHub previews terminology in attestation commands' help docs by [@​BagToad](https://github.com/BagToad) in cli/cli#9799 - Clarify in README that `gh` is supported on GitHub Enterprise Cloud by [@​BagToad](https://github.com/BagToad) in cli/cli#9805 - build(deps): bump github.com/gabriel-vasile/mimetype from 1.4.5 to 1.4.6 by [@​dependabot](https://github.com/dependabot) in cli/cli#9752 ##### Acceptance Test Changes - Add acceptance tests for `workflow`, `run`, and `cache` commands by [@​BagToad](https://github.com/BagToad) in cli/cli#9766 - Add basic `api` acceptance tests by [@​BagToad](https://github.com/BagToad) in cli/cli#9770 - Add acceptance tests for `release` commands by [@​BagToad](https://github.com/BagToad) in cli/cli#9771 - Add acceptance tests for `org` and `ssh-key` commands by [@​BagToad](https://github.com/BagToad) in cli/cli#9812 - Add acceptance tests for `gh auth` commands by [@​jtmcg](https://github.com/jtmcg) in cli/cli#9787 - Add acceptance tests for `repo` commands by [@​jtmcg](https://github.com/jtmcg) in cli/cli#9783 - Add acceptance tests for `search` command by [@​BagToad](https://github.com/BagToad) in cli/cli#9786 - Add acceptance tests for `variable` commands by [@​andyfeller](https://github.com/andyfeller) in cli/cli#978 - Add testscripts for gpg-key and label commands by [@​williammartin](https://github.com/williammartin) in cli/cli#9811 - Use forked testscript for token redaction by [@​williammartin](https://github.com/williammartin) in cli/cli#9804 - Add acceptance tests for `secret` commands by [@​andyfeller](https://github.com/andyfeller) in cli/cli#9782 - Note token redaction in Acceptance test README by [@​williammartin](https://github.com/williammartin) in cli/cli#9813 #### New Contributors - [@​tsukasaI](https://github.com/tsukasaI) made their first contribution in cli/cli#9790 **Full Changelog**: cli/cli@v2.59.0...v2.60.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
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.
Fixes #7804
Fixes #9325
Requirement
allow
gh pr create --web --template=[template file]to create a PR using the specified templateSolution
IssueMetadataStategiven the query is allowed in both Issue and Pull Request creation