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 pr-status command #67

Merged
merged 1 commit into from
May 18, 2023
Merged

Add pr-status command #67

merged 1 commit into from
May 18, 2023

Conversation

rnorth
Copy link
Collaborator

@rnorth rnorth commented Oct 3, 2021

Fixes #18, #26
Alternative solution to #58

Example:

$ turbolift pr-status --list
  OK   Reading campaign data
  OK   Checking PR status for rnorth/turbolift-play-1
  OK   Checking PR status for rnorth/turbolift-play-2
 WARN  Checking PR status for rnorth/turbolift-play-3: Directory work/rnorth/turbolift-play-3 does not exist - has it been cloned?

  OK   turbolift pr-status completed


Repository               State   Reviews  URL
rnorth/turbolift-play-1  OPEN             https://github.com/rnorth/turbolift-play-1/pull/1
rnorth/turbolift-play-2  CLOSED           https://github.com/rnorth/turbolift-play-2/pull/1

State      Count
Merged     0
Open       1
Closed     1
Skipped    1
Not Found  0

Reaction  Count
👍         1
👎         2

TODO:

  • Tidy up presentation
  • Add review status
  • ???

@rnorth
Copy link
Collaborator Author

rnorth commented Oct 3, 2021

@sledigabel @JimNero009 what do you think?

@rnorth rnorth changed the title Upgrade golangci-lint Add pr-status command Oct 3, 2021
@sledigabel
Copy link
Contributor

just some tiny comments, overall I'm loving it :-)

@rnorth
Copy link
Collaborator Author

rnorth commented Jan 11, 2022

Updated output:

$ turbolift pr-status
  OK   Reading campaign data
  OK   Checking PR status for rnorth/turbolift-play-1
  OK   Checking PR status for rnorth/turbolift-play-2
  OK   turbolift pr-status completed


State      Count
Merged     0
Open       2
Closed     0
Skipped    0
Not Found  0

Reactions: 👍 1   🎉 1   ❤️ 1

@rnorth
Copy link
Collaborator Author

rnorth commented Jan 11, 2022

@sledigabel Ah for verifying the review status column, I think we'll need to run this against repos where someone can approve a PR. I have some private repos for playing in, but obviously can't approve my own PRs there 😂

@rnorth rnorth marked this pull request as ready for review January 11, 2022 21:29
Copy link
Contributor

@sledigabel sledigabel left a comment

Choose a reason for hiding this comment

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

A few comments, overall it looks great, well done @rnorth !

sledigabel added a commit that referenced this pull request Jan 26, 2022
Relates to #65
Fixes #60
Relates to #67

This PR implements a global PR closing command.
It introduces a new `update-prs` verb which has only one option
at the moment: `--close`.

It uses the same mechanism as in #67, looking at the PRs and identifying
the one related to the campain, then closes it.

It handles the case where there is no open PR.
@rnorth rnorth marked this pull request as draft April 24, 2022 08:46
@rnorth
Copy link
Collaborator Author

rnorth commented Apr 24, 2022

Marked as draft until #72 is in.

rnorth added a commit that referenced this pull request Jun 27, 2022
* Adds `update-prs` with `close` feature

Relates to #65
Fixes #60
Relates to #67

This PR implements a global PR closing command.
It introduces a new `update-prs` verb which has only one option
at the moment: `--close`.

It uses the same mechanism as in #67, looking at the PRs and identifying
the one related to the campain, then closes it.

It handles the case where there is no open PR.

* Update after PR comments

- typos
- Readme addition

Signed-off-by: Sebastien Le Digabel <sledigabel@gmail.com>

Co-authored-by: Richard North <rich.north@gmail.com>
@rnorth rnorth marked this pull request as ready for review August 9, 2022 12:17
@rnorth rnorth marked this pull request as draft August 15, 2022 08:11
@rnorth
Copy link
Collaborator Author

rnorth commented Aug 15, 2022

Ah, the GetPR function refuses to return a PR which has status closed - which is a problem, as merged PRs now come back with a No PR Found error.

@sledigabel do you remember if there's such a strong need for GetPR to filter out closed PRs?

i.e. in:

    // if the user has write permissions on the repo,
	// the PR should be under _CurrentBranch_.
	if prr.CurrentBranch != nil && !prr.CurrentBranch.Closed {
		return prr.CurrentBranch, nil
	}

	// If not, then it's a forked PR. The headRefName is as such: `username:branchName`
	for _, pr := range prr.CreatedBy {
		if strings.HasSuffix(pr.HeadRefName, branchName) && !pr.Closed {
			return pr, nil
		}
	}

	return nil, &NoPRFoundError{Path: workingDir, BranchName: branchName}

This code is rejecting the below status response, for example:

{
  "createdBy": [],
  "currentBranch": {
    "closed": true,
    "headRefName": "xxxx",
    "mergeable": "UNKNOWN",
    "number": 88,
    "reactionGroups": [],
    "reviewDecision": "APPROVED",
    "state": "MERGED",
    "title": "xxxx",
    "url": "xxxx/pull/88"
  },
  "needsReview": []
}

@sledigabel
Copy link
Contributor

@rnorth Upon a very late check, I don't think the pr.Closed check is required at all.
The expectation when GetPR came up was that the PR would be non existant or open I think.
I believe it can be safely removed.

@rnorth
Copy link
Collaborator Author

rnorth commented Oct 6, 2022

Awesome, thanks for clarifying @sledigabel!

@rnorth
Copy link
Collaborator Author

rnorth commented Oct 6, 2022

Now producing far more sensible output, e.g:

...
  OK   turbolift pr-status completed


State        Count
Merged       139
Open         53
Closed       29
Skipped      0
No PR Found  1

@rnorth rnorth marked this pull request as ready for review October 6, 2022 15:24
@rnorth
Copy link
Collaborator Author

rnorth commented Oct 6, 2022

The --list mode looks pretty reasonable, I think, too:

...
  OK   turbolift pr-status completed


Repository                                                State   Reviews            URL
redacted/redacted                                         OPEN    REVIEW_REQUIRED    https://github.redacted/redacted/redacted/pull/262
redacted/redacted                                         OPEN    REVIEW_REQUIRED    https://github.redacted/redacted/redacted/pull/515
redacted/redacted                                         OPEN    REVIEW_REQUIRED    https://github.redacted/redacted/redacted/pull/342
redacted/redacted                                         MERGED  APPROVED           https://github.redacted/redacted/redacted/pull/407
redacted/redacted                                         MERGED  REVIEW_REQUIRED    https://github.redacted/redacted/redacted/pull/220
redacted/redacted                                         OPEN    REVIEW_REQUIRED    https://github.redacted/redacted/redacted/pull/105
redacted/redacted                                         MERGED  APPROVED           https://github.redacted/redacted/redacted/pull/532
redacted/redacted                                         MERGED  APPROVED           https://github.redacted/redacted/redacted/pull/268
redacted/redacted                                         OPEN    REVIEW_REQUIRED    https://github.redacted/redacted/redacted/pull/438
...

Copy link
Contributor

@catscanner catscanner left a comment

Choose a reason for hiding this comment

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

adds a command to show the current state of turbolift raised prs

@rnorth rnorth merged commit 0e1678b into Skyscanner:main May 18, 2023
@rnorth rnorth deleted the pr-status branch May 18, 2023 16:25
sledigabel added a commit that referenced this pull request May 24, 2023
The `pr-status` command was added after and needed the extra danse to
include the new repo file option.

Also renamed the `pr_status` package into `prstatus` as gofumpt told me
packages should not include underscores.
https://go.dev/blog/package-names
sledigabel added a commit that referenced this pull request Jun 19, 2023
* Optional repo file

Fixes #74

Adds a new flag for all commands `--repos=<filename>`, which gives the
ability to select a different file from the default `repos.txt` one.

* Fixing defaultOptions naming

* Renaming test

* Fix typo on CampaignOptions

* Updates following PR

- Changed error text to include filename last

Signed-off-by: Sebastien Le Digabel <sledigabel@gmail.com>

* Adding repofile to update_pr

* Rebasing after #67

The `pr-status` command was added after and needed the extra danse to
include the new repo file option.

Also renamed the `pr_status` package into `prstatus` as gofumpt told me
packages should not include underscores.
https://go.dev/blog/package-names

* Changed the campaign start message with the repo filename

.. addressing the PR comment.
Also renamed the update_prs package name for the same reason as the
pr_status one.

* Fixing the parsing for foreach

Adding a manual parsing of the foreach parameters and extra testing.
Also added some details in the Readme

* Rephrasing comment in foreach

* Removing extraneous comment

---------

Signed-off-by: Sebastien Le Digabel <sledigabel@gmail.com>
Co-authored-by: Cat from Catalyst <catscanner@users.noreply.github.com>
Co-authored-by: Richard North <rich.north@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

turbolift status command
3 participants