Skip to content
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

fix: expose run_attempt field (as attempt) when getting a workflow run #8905

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

cawfeecake
Copy link
Contributor

the run_attempt field is not exposed in the run view and run ls commands, so the only why to get them via the cli is to use gh api

this change exposes it, but also renames the current field that wasn't being used as the data the field contains is not "how many attempts" rather "which attempt this info is for"

@cawfeecake cawfeecake requested a review from a team as a code owner April 1, 2024 20:45
@cawfeecake cawfeecake requested review from andyfeller and removed request for a team April 1, 2024 20:45
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 1, 2024
@cliAutomation
Copy link
Collaborator

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Apr 1, 2024
@andyfeller
Copy link
Contributor

@cawfeecake : While we appreciate contributors improving the GitHub CLI, could you create an issue that we might discuss this need outside of the PR?

@@ -97,7 +98,7 @@ type Run struct {
workflowName string // cache column
WorkflowID int64 `json:"workflow_id"`
Number int64 `json:"run_number"`
Attempts uint64 `json:"run_attempt"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm genuinely surprised there is no other code that uses this field since it was added in #5945 🤯

That said, how should test cases within the following be updated to ensure this is being exported?

func TestRunExportData(t *testing.T) {

Copy link
Member

Choose a reason for hiding this comment

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

I'm genuinely surprised there is no other code that uses this field since it was added in #5945 🤯

I also went down this rabbit hole like gahhh. It's funny because it was even updated to use uint64 instead of 8 bits because it was causing an unmarshal error even tho it was never useeeeeed.

@williammartin
Copy link
Member

@cawfeecake please create an issue that describes your use case for this field to help us understand the motivation. It's very useful for us and the rest of the community to see what outcomes people are using the CLI for.

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
No open projects
The GitHub CLI
  
Needs review 🤔
Development

Successfully merging this pull request may close these issues.

None yet

4 participants