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

Add :Octo pr runs command #911

Merged
merged 4 commits into from
Mar 2, 2025
Merged

Add :Octo pr runs command #911

merged 4 commits into from
Mar 2, 2025

Conversation

wd60622
Copy link
Collaborator

@wd60622 wd60622 commented Mar 2, 2025

Describe what this PR does / why we need it

Expose the branch in the list function which is used in new command :Octo pr runs to get the runs associated with the PR.

In this case, the value in the picker are all the same. Would it make sense to show the workflow name instead of the branch name? That is what comes from gh run list --branch pr-runs --json workflowName

For instance, this run would be labeled "Sync Closing Labels"

CC: @GustavEikaas

Does this pull request fix one issue?

Closes #910

Describe how you did it

Describe how to verify it

Special notes for reviews

Checklist

  • Passing tests and linting standards
  • Documentation updates in README.md and doc/octo.txt

@wd60622 wd60622 added command Dealing with Octo commands enhancement New feature or request labels Mar 2, 2025
Copy link
Contributor

@GustavEikaas GustavEikaas left a comment

Choose a reason for hiding this comment

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

Looks good!

One thing however;

You are passing branch to the filter for displaying workflows related to a pr. This will return all workflows run on this branch. Including the ones that have been run with triggers like

workflow_dispatch:

Is that intentional?

EDIT:
It makes sense IMO just sanity checking.

@GustavEikaas
Copy link
Contributor

GustavEikaas commented Mar 2, 2025

In this case, the value in the picker are all the same. Would it make sense to show the workflow name instead of the branch name? That is what comes from gh run list --branch pr-runs --json workflowName
For instance, this run would be labeled "Sync Closing Labels"

Yes definitely, very confusing when you list wf runs for a PR and they are all named the same

EDIT: I would use the json field name to get the run-name instead of the workflow name
The name property seems to take the workflowName if there is no run-name defined
image
image

@wd60622
Copy link
Collaborator Author

wd60622 commented Mar 2, 2025

I was playing around with it and came to same conclusion in 9aec991

@wd60622
Copy link
Collaborator Author

wd60622 commented Mar 2, 2025

You are passing branch to the filter for displaying workflows related to a pr. This will return all workflows run on this branch. Including the ones that have been run with triggers like

workflow_dispatch:

Is that intentional?

Yeah, I think that is probably fine

@wd60622
Copy link
Collaborator Author

wd60622 commented Mar 2, 2025

I think it might make sense to have name even for the larger run list? What are your thoughts? Maybe prepend name to what is already being displayed? Bit separate so can address that some other time..

@GustavEikaas
Copy link
Contributor

GustavEikaas commented Mar 2, 2025

What we could do as an overall improvement is this. This would allow you to easily see the branch name it was executed on without scrolling

 local display
      if opts.branch == nil then
        display = string.format("%s (%s), value.name, value.headBranch)
      else
        display = value.name
      end

Kinda similar to how the github web UI does it
image

@GustavEikaas
Copy link
Contributor

Awesome! LGTM🚀

@wd60622
Copy link
Collaborator Author

wd60622 commented Mar 2, 2025

I've added that in in 483e424 . I tend to search by the "name" on the workflow

Let me know how this looks to you

@wd60622
Copy link
Collaborator Author

wd60622 commented Mar 2, 2025

Thanks for the feedback @GustavEikaas !

@GustavEikaas
Copy link
Contributor

Remember to update readme

@wd60622 wd60622 merged commit ad15e6f into pwntester:master Mar 2, 2025
3 checks passed
@wd60622 wd60622 deleted the pr-runs branch March 2, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command Dealing with Octo commands enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get workflow runs for a PR
2 participants