Skip to content

Add branch and actor filters to run list#4100

Merged
samcoe merged 6 commits intocli:trunkfrom
bak1an:run-list-filters
Jan 26, 2022
Merged

Add branch and actor filters to run list#4100
samcoe merged 6 commits intocli:trunkfrom
bak1an:run-list-filters

Conversation

@bak1an
Copy link
Contributor

@bak1an bak1an commented Aug 8, 2021

Both https://docs.github.com/en/rest/reference/actions#list-workflow-runs and https://docs.github.com/en/rest/reference/actions#list-workflow-runs-for-a-repository accept those query parameters and seems that it would be handy to have it in cli as well.


upd: Just noticed that there is a related issue #3686, have not seen it initially when opening the PR. Will pause adding tests until getting some initial feedback on the approach taken.

@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.

@mislav mislav requested review from a team and mislav and removed request for a team August 10, 2021 13:21
@sarcasticadmin
Copy link

sarcasticadmin commented Oct 14, 2021

Anything that still needs to be done before we can get this merged? Id love to utilize this :)

@bak1an
Copy link
Contributor Author

bak1an commented Nov 17, 2021

@mislav Can this be moved forward somehow?

@mislav mislav added the external pull request originating outside of the CLI core team label Dec 20, 2021
@vilmibm vilmibm assigned samcoe and unassigned mislav Jan 10, 2022
@vilmibm vilmibm requested a review from samcoe January 10, 2022 17:09
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

@bak1an Thanks for the contribution and sorry for the delay getting this reviewed. Overall really like the approach here I just left one comment about reducing the responsibilities of FilterOptions. Are you still open to adding tests for this? Seems like you were waiting for direction approval before adding those.

@bak1an
Copy link
Contributor Author

bak1an commented Jan 13, 2022

@samcoe Thanks for a review! I am still around and can update the PR and will try adding tests for this.

@bak1an bak1an requested a review from a team as a code owner January 22, 2022 21:34
@bak1an
Copy link
Contributor Author

bak1an commented Jan 22, 2022

@samcoe Hi. The PR is now rebased onto fresh trunk, FilterOptions does not do anything smart and added some tests.

Added checks in TestNewCmdList that corresponding command line parameters are properly stored into ListOptions.

Also checking in TestListRun that stubbed API requests do have corresponding query parameters when they are set in given ListOptions. I added ad hoc httpmock.Matcher right inside the test to do that. Was not sure if it will be useful for anything else and decided not to touch httpmock itself to keep this PR more about runs filtering.

Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

@bak1an This is great! Thanks for being thorough with the tests. I like the new query string matcher a lot and I can see it being used in the future. When that happens we can extract it into httpmock. I pushed one small commit to just change the name of the flag from actor to user. Thanks for your contribution.

@bak1an
Copy link
Contributor Author

bak1an commented Jan 24, 2022

Thanks @samcoe 🎉

@samcoe samcoe merged commit e28fa3c into cli:trunk Jan 26, 2022
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.

6 participants