-
Notifications
You must be signed in to change notification settings - Fork 7.1k
feat: add support for --ref
in gh cache delete
#11592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
daf902a
to
0d1629a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for a --ref
flag to the gh cache delete
command, allowing users to narrow down cache deletion to a specific Git reference (branch or PR). The enhancement enables more precise cache management by specifying refs like refs/heads/branch-name
or refs/pull/123/merge
.
- Added
--ref
flag to cache delete command with validation logic - Updated API integration to include ref parameter in delete requests
- Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
pkg/cmd/cache/delete/delete.go | Implements --ref flag with validation and API parameter handling |
pkg/cmd/cache/delete/delete_test.go | Adds test cases for --ref flag validation and API integration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution, @luxass! 🫶
This looks pretty good with some minor suggestions to ensure users have clear guidance.
- Introduced `--ref` flag to narrow down cache deletion to a specific branch or PR reference. - Updated command examples to include usage of `--ref`. - Added validation to ensure `--ref` cannot be used with `--all` and must accompany a cache key/ID.
* Implemented test cases for handling the `--ref` flag in cache deletion commands. * Added validation for using `--ref` with cache key and ID. * Ensured proper error messages are returned for invalid usage of `--ref`.
* Clarified the description of the `--ref` flag to specify its purpose more accurately. * Ensures users understand that it narrows down deletion by cache key and ref.
* Changed the error message from "--ref cannot be used without cache key/ID" to "must provide a cache key" for clarity. * Updated corresponding test case to reflect the new error message.
f2102ad
to
047326f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks very good, @luxass! 🎉
To finalise the changes, I think we need to cover scenarios below. When you're done I'll go through the testing part.
1. Use --ref
with a cache ID
The --ref
option only applies to cases where the argument is a cache key. Therefore we need to add another validation check to make sure --ref
is not used with a cache ID, with an error message like this:
--ref cannot be used with a cache ID
For this we can extract a small function like below and use it in both cmd.RunE
(for validation) and deleteCaches
.
func parseCacheID(arg string) (int, bool) {
id, err := strconv.Atoi(arg)
return id, err == nil
}
2. Better error message when --ref
not found
Currently, if the user runs cache delete <cache-key> --ref <ref>
where ref
is invalid (or doesn't exist) then they'll see an error like this:
X Could not find a cache matching <cache-key> in <repo>
Since the user has provided the --ref
option I think we should hint at the ref value in the error message. Something like this makes sense to me:
X Could not find a cache matching <cache-key> (with ref <ref>) in <repo>
Note that this format should only be used when there's a non-empty --ref
value.
* Replaced direct conversion of cache ID string to int with a dedicated `parseCacheID` function. * Improved readability and maintainability of the `deleteCaches` function.
* Added validation to ensure that the `--ref` flag cannot be used when a cache ID is provided. * Updated error message to include the reference when a cache is not found with the specified `--ref`.
* Ensure that the `--ref` flag cannot be used in conjunction with a cache ID. * Update error handling to provide clear feedback when this condition is violated. * Add tests to cover scenarios involving the `--ref` flag and cache ID.
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Also note that the API never returns HTTP 422 for invalid refs. Signed-off-by: Babak K. Shandiz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, @luxass! 🙏
I went through the manual testing and noticed a few things. I'll now push some commits to fix the comments I just made below. With those, I think the PR is ready to be merged, but I'll ask for a another review from @andyfeller since I have made some changes.
@andyfeller I just resolved the comments from my last review since they're all sorted in the commits I pushed. I think the PR is good to be merged, but will appreciate your eyes on it. 🙏 |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.78.0` -> `v2.79.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.79.0`](https://github.com/cli/cli/releases/tag/v2.79.0): GitHub CLI 2.79.0 [Compare Source](cli/cli@v2.78.0...v2.79.0) #### Advanced Issue Search Support The GitHub CLI now supports advanced issue search syntax using: - Searching issues: `gh search issues <advanced issue search query>` - Searching pull requests: `gh search prs <advanced issue search query>` - While listing issues: `gh issue list --search <advanced issue search query>` - While listing pull requests: `gh pr list --search <advanced issue search query>` For more information about advanced issue search syntax, see: "[Filtering and Searching Issues and Merge Requests](https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/filtering-and-searching-issues-and-pull-requests#building-advanced-filters-for-issues)" #### Copy OAuth Code Automatically The GitHub CLI now supports writing the OAuth one-time pass code to the clipboard automatically during authentication: - While logging in: `gh auth login --clipboard` / `gh auth login -c` - While refreshing the token: `gh auth refresh --clipboard` / `gh auth refresh -c` #### What's Changed ##### ✨ Features - feat: `gh auth` Automatically copy one-time OAuth code to clipboard by [@​ankddev](https://github.com/ankddev) in [#​11518](cli/cli#11518) - feat: add support for `--ref` in `gh cache delete` by [@​luxass](https://github.com/luxass) in [#​11592](cli/cli#11592) - Use advanced issue search by [@​babakks](https://github.com/babakks) in [#​11638](cli/cli#11638) ##### 📚 Docs & Chores - docs(release create): difference `--generate-notes` and `--notes-from-tag` by [@​ankddev](https://github.com/ankddev) in [#​11534](cli/cli#11534) - refactor tests: use `slices.Equal` to simplify code by [@​minxinyi](https://github.com/minxinyi) in [#​11364](cli/cli#11364) - Remove mention of public preview in trustedroot.go by [@​jkylekelly](https://github.com/jkylekelly) in [#​11652](cli/cli#11652) #####Dependencies - Bump sigstore/rekor to v1.4.1 by [@​BagToad](https://github.com/BagToad) in [#​11654](cli/cli#11654) - chore(deps): bump actions/stale from 9 to 10 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​11663](cli/cli#11663) - chore(deps): bump actions/setup-go from 5 to 6 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​11662](cli/cli#11662) #### New Contributors - [@​minxinyi](https://github.com/minxinyi) made their first contribution in [#​11364](cli/cli#11364) - [@​jkylekelly](https://github.com/jkylekelly) made their first contribution in [#​11652](cli/cli#11652) - [@​luxass](https://github.com/luxass) made their first contribution in [#​11592](cli/cli#11592) **Full Changelog**: <cli/cli@v2.78.0...v2.79.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:eyJjcmVhdGVkSW5WZXIiOiI0MS45OC4xIiwidXBkYXRlZEluVmVyIjoiNDEuOTguMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
resolves #11425