Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix unhandled panic in FindWorkflow and add tests - #10521

#10521
Merged
andyfeller merged 1 commit into
trunkcli/cli:trunkfrom
jtmcg/fix-workflow-paniccli/cli:jtmcg/fix-workflow-panicCopy head branch name to clipboard
Mar 3, 2025
Merged

Fix unhandled panic in FindWorkflow and add tests#10521
andyfeller merged 1 commit into
trunkcli/cli:trunkfrom
jtmcg/fix-workflow-paniccli/cli:jtmcg/fix-workflow-panicCopy head branch name to clipboard

Conversation

@jtmcg

@jtmcg jtmcg commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

Fixes #10519

Changes

  • Handle panic in FindWorkflows
  • Add tests to cover FindWorkflows, getWorkflowByID, getWorkflowsByName, and GetWorkflows

Notes

FindWorkflow had a code path that could error without handling that error that was introduced in #10328. I previously called out the lack of tests in this part of the code in my review, and punting them then bit us here: #10328 (review).

As such, this fixes the panic, but also adds tests to cover the relevant code paths through FindWorkflow, including some tests for getWorkflowByID, getWorkflowsByName, and GetWorkflows.

This does NOT seek to refactor the existing code, just cover the existing behavior and validate that we are resolving this panic (covered by TestFindWorkflow_nonHTTPError).

Testing

The panic has been hard to reproduce, as can be seen on the issue, and we're not actually sure what is causing the error. However, I was able to create a situation to repro the panic in TestFindWorkflow_nonHTTPError:

$ go test
$ go test -run ^TestFindWorkflow_nonHTTPError$
--- FAIL: TestFindWorkflow_nonHTTPError (0.00s)
    --- FAIL: TestFindWorkflow_nonHTTPError/When_the_client_fails_to_instantiate,_it_returns_the_error (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x100636de8]

goroutine 8 [running]:
testing.tRunner.func1.2({0x10072fb60, 0x1009fd9d0})
        /usr/local/go/src/testing/testing.go:1632 +0x1bc
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1635 +0x334
panic({0x10072fb60?, 0x1009fd9d0?})
        /usr/local/go/src/runtime/panic.go:785 +0x124
github.com/cli/cli/v2/pkg/cmd/workflow/shared.FindWorkflow(0x14000109f20, {0x10079e2d8, 0x14000199e30}, {0x1006e1a70, 0x1}, {0x0, 0x0, 0x0})
        /Users/tylermcgoffin/projects/cli/cli/pkg/cmd/workflow/shared/shared.go:139 +0x148
github.com/cli/cli/v2/pkg/cmd/workflow/shared.TestFindWorkflow_nonHTTPError.func1(0x14000119040)
        /Users/tylermcgoffin/projects/cli/cli/pkg/cmd/workflow/shared/shared_test.go:180 +0x150
testing.tRunner(0x14000119040, 0x100797cf0)
        /usr/local/go/src/testing/testing.go:1690 +0xe4
created by testing.(*T).Run in goroutine 7
        /usr/local/go/src/testing/testing.go:1743 +0x314
exit status 2
FAIL    github.com/cli/cli/v2/pkg/cmd/workflow/shared   0.167s

Thus, I think just verifying the tests pass is the only testing needed.

FindWorkflow had a code path that could error without handling that error
that was introduced in #10328. I previously called out the lack of tests
in this part of the code in my review, and punting them then bit us here:
#10328 (review).

As such, this fixes the panic, but also adds tests to cover the relevant
code paths through FindWorkflow, including some tests for getWorkflowByID,
getWorkflowsByName, and GetWorkflows.

This does NOT seek to refactor the existing code, just cover the existing
behavior and validate that we are resolving this panic (covered by
TestFindWorkflow_nonHTTPError).
@jtmcg
jtmcg requested a review from a team as a code owner February 28, 2025 23:22
@jtmcg
jtmcg requested a review from andyfeller February 28, 2025 23:22
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Feb 28, 2025

@andyfeller andyfeller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

expectedWorkflows: []Workflow(nil),
},
{
name: "When there are more than one workflow with the same name, only the ones matching the provided state are returned",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Previously, I haven't considered whether Actions will allow you to have the same workflow with multiple names and states, however I just confirmed it does.

neat-bender

@andyfeller
andyfeller merged commit 5e21f6a into trunk Mar 3, 2025
@andyfeller
andyfeller deleted the jtmcg/fix-workflow-panic branch March 3, 2025 18:14
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Mar 6, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cli/cli](https://github.com/cli/cli) | minor | `v2.67.0` -> `v2.68.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.68.0`](https://github.com/cli/cli/releases/tag/v2.68.0): GitHub CLI 2.68.0

[Compare Source](cli/cli@v2.67.0...v2.68.0)

#### What's Changed

##### ✨ Features

-   \[gh repo view] Improve error message for forked repo by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10334
-   Add signer-digest, source-ref, and source-digest options for `gh attestation verify` by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10308
-   \[gh pr checkout] Add --no-tags option to git fetch commands in checkout by [@&#8203;latzskim](https://github.com/latzskim) in cli/cli#10479
-   \[`gh issue/pr comment`] Add `--create-if-none` and prompts to create a comment if no comment already exists  by [@&#8203;latzskim](https://github.com/latzskim) in cli/cli#10427
-   \[gh cache delete --all] Add `--succeed-on-no-caches` flag to return exit code 0 by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10327
-   \[gh release create] Fail when there are no new commits since the last release by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10398
-   update default upstream when forking repo during MR creation by [@&#8203;daviddl9](https://github.com/daviddl9) in cli/cli#10458

##### 🐛 Fixes

-   Refactor `GetLocalAttestations` and clean up custom registry transport by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10382
-   Check `GH_REPO` too in addition to `--repo` for disambiguation by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10539
    -   (Fixes `gh secret` subcommands not working outside of a repository)
-   Fix unhandled panic in FindWorkflow and add tests by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10521
-   Fix checkout when URL arg is from fork and cwd is upstream by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10512
-   \[gh api] Escape package name (URL encoding) for packages endpoint by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10384
-   Fix `remoteResolver` caching issue by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10456
-   Fix gh project item-edit to allow --number 0 as a valid value by [@&#8203;aryanbhosale](https://github.com/aryanbhosale) in cli/cli#10417
-   Add mutex to fix race in attestation test client by [@&#8203;codysoyland](https://github.com/codysoyland) in cli/cli#10439
-   Base64 decode GPG passphrase in deployment workflow by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10546

##### 📚 Docs & Chores

-   Deep Dive Document Release Process by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10503
-   Inconsistent format of examples in help text by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10508
-   Inconsistent format of description of flags (starting with lowercase letter) by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10507
-   Update Go version to 1.23 in CONTRIBUTING.md by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10504
-   Fix minor auth login help typo by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10501
-   docs: document how to revoke `gh` OAuth tokens in `auth logout`'s help by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10490
-   chore: update codespaces Go version by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10491
-   Allow injection of TUFMetadataDir in tests by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10478
-   refactor: use a more straightforward return value by [@&#8203;beforetech](https://github.com/beforetech) in cli/cli#10489
-   Use subtests in attestation verification integration tests by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10463
-   Fix typo in README by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10445
-   Update usage to lower-kebab-case by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10447
-   Standardize URLs by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10429
-   Remove trailing whitespace by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10430

##### :dependabot: Dependencies

-   Bump actions/attest-build-provenance from 2.2.0 to 2.2.2 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10518
-   Bump github.com/go-jose/go-jose/v4 from 4.0.2 to 4.0.5 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10499
-   Bump github.com/spf13/pflag from 1.0.5 to 1.0.6 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10338

</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:eyJjcmVhdGVkSW5WZXIiOiIzOS4xODYuMCIsInVwZGF0ZWRJblZlciI6IjM5LjE4Ni4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic using gh workflow run in GitHub Actions

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.