Skip to content

Feature: Comment on released PRs and issues - #80

Open
ayshiff wants to merge 8 commits into
changesets:mainfrom
ayshiff:feature/release-comment
Open

Feature: Comment on released PRs and issues#80
ayshiff wants to merge 8 commits into
changesets:mainfrom
ayshiff:feature/release-comment

Conversation

@ayshiff

@ayshiff ayshiff commented Mar 22, 2021

Copy link
Copy Markdown

Closes #511

As seen with @mitchellhamilton, this PR is a duplicate of #25 and has been moved here to make the feature more configurable and consistent.

This PR adds a new feature that allows to comment on PRs and issues that have been released.
It also adds a new input comment which allows to activate or not the feature.

My work is inspired by what semantic-release/github has done.

Here is a schema showing the workflow to retrieve the released Pull Requests and Issues.

Comment message

Here is a comment message example:

🦋 This work has been released in release version: v0.4.3

Release link: https://github.com/backstage/backstage/releases/tag/v0.4.3

Note that the message is the same for PRs and issues.
It might be interesting to have two different messages for the different cases?

Example

I also created a codesanbox where you can see the logic to get the issues and pull-requests associated to a release more easily.
NOTE: In the codesanbox, considering that the code does not run at the time the release is created, we will get more recent commits. This will not happen in the real world.

Notes

I couldn't find a better way to find the tag associated to the release than looking through the tags until I found the one corresponding to the release.

@ayshiff
ayshiff force-pushed the feature/release-comment branch from 648861b to 79c9dda Compare March 23, 2021 10:11
@emmatown

Copy link
Copy Markdown
Member

Could you create a repo and use your fork of the action on it and test it out so we can see what it looks like?

Comment thread src/run.ts Outdated
createRelease(octokit, {
pkg,
tagName: `${pkg.packageJson.name}@${pkg.packageJson.version}`,
comment: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why this is hardcoded to false? 🤔 shouldn't we use a configured value here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is hardcoded to false because we want to comment on whole repository release rather than on each package release.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, there is no such thing as "whole repository release" if I recall correctly. The other createRelease is for single-package repositories but in monorepos we are just looping through all released packages so there is 1 to 1 relation between released packages and created releases (no extra repository-wide release)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh right I misunderstood what @mitchellhamilton meant here changesets/bot#25 (review)

I'll use the comment value from the input property.

Comment thread src/run.ts
import * as gitUtils from "./gitUtils";
import readChangesetState from "./readChangesetState";
import resolveFrom from "resolve-from";
import issueParser from "issue-parser";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

q: in the GitHub interface we have "Linked issues", I assume that this list is received by their frontend from their backend and that they don't have to prepare the PR body to get that. Would it be possible to leverage that? Or are there some limitations regarding this technique? One thing that comes to my mind - the list might include more than we want but maybe it's associated with some metadata that we could leverage to filter that list?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree with you, I wish I had found a more appropriate solution but I think we don't have the necessary information in the API response (see https://docs.github.com/en/rest/reference/pulls#get-a-pull-request).

I tried with a Pull Request that has several related issues and they don't appear in the API response.

@Andarist Andarist Mar 24, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There seems to be an indirect way using the GraphQL API: https://github.community/t/get-all-issues-linked-to-a-pull-request/14653/6

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doc also claims that it's possible to query that ingo using filters: https://docs.github.com/en/github/searching-for-information-on-github/searching-issues-and-pull-requests#search-for-linked-issues-and-pull-requests

But I couldn't make it work to get actual issue numbers.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

fragment IssueOrPullRequestData on ReferencedSubject {
  ... on Issue {
    number
    body
    issueState: state
  }
  ... on PullRequest {
    number
    state
  }
}

{
  resource(url: "https://github.com/graphql/graphiql/pull/1914") {
    ... on PullRequest {
      timelineItems(itemTypes: [
        CONNECTED_EVENT, 
        DISCONNECTED_EVENT, 
        CROSS_REFERENCED_EVENT, 
        REFERENCED_EVENT
			], first: 100) {
        nodes {
          __typename
          ... on ReferencedEvent {
            id
            actor {
              url
            }
            isCrossRepository
            isDirectReference
            subject {
              ...IssueOrPullRequestData
            }
          }
          ... on DisconnectedEvent {
            id
            subject {
              ...IssueOrPullRequestData
            }
          }
          ... on CrossReferencedEvent {
            resourcePath
            isCrossRepository
            willCloseTarget
            target {
              ...IssueOrPullRequestData
            }
            source {
              ...IssueOrPullRequestData
            }
          }
        }
      }
    }
  }
}

got a little closer, but not quite. using this query, I was able to get the PR referenced from a referenced issue, but not the referenced issue itself 😆 ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can now be queried with:

{
  resource(url: "https://github.com/graphql/graphiql/pull/1914") {
    ... on PullRequest {
      closingIssuesReferences(first: 99) {
        nodes {
          number
        }
      }
    }
  }
}

I will be revamping this PR this week to hopefully land it before the end of the year.

Comment thread src/run.ts Outdated
Comment on lines +74 to +96
/*
Here are the following steps to retrieve the released PRs and issues.

1. Retrieve the tag associated with the release
2. Take the commit sha associated with the tag
3. Retrieve all the commits starting from the tag commit sha
4. Retrieve the PRs with commits sha matching the release commits
5. Map through the list of commits and the list of PRs to
find commit message or PRs body that closes an issue and
get the issue number.
6. Create a comment for each issue and PR
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Each released thing should be associated with a changeset. I know that it's still possible to just release something while piggybacking on the already prepared release and close something that way but... should we care about it? Shouldn't we guide users to the best practices?

What if we'd make this changeset-based and just appropriately mix that info with additional data (like the link to the release)? We could also add contents of the relevant changesets to the generated comment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree with that!

So the idea would be to do that in the runVersion() method when we have changesets.
In the readChangesetState() response we can retrieve the changeset releases:

// changesets: { id, summary, releases }[]
let { preState, changesets } = await readChangesetState(cwd);

We can then base our logic on these changesets releases.

@ayshiff

ayshiff commented Mar 24, 2021

Copy link
Copy Markdown
Author

Could you create a repo and use your fork of the action on it and test it out so we can see what it looks like?

Here is an example:

Comment thread src/run.ts Outdated
body: getReleaseMessage(htmlUrl, tagName),
};

octokit.issues.createComment(issueComment);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
octokit.issues.createComment(issueComment);
return octokit.issues.createComment(issueComment);

Otherwise the outer Promise.all won't wait on those calls.

@emmatown

Copy link
Copy Markdown
Member

Could you make your test repo have a bunch of packages, I'm curious to see what that looks like since there's no single release on GH to link to?

@Andarist

Copy link
Copy Markdown
Member

@mitchellhamilton since creating those comments is done currently just after creating a release then this has to "spam" issues, creating a comment for each release from that single run. This definitely needs improvement and we should aggregate stuff to create at most one comment per issue.

@ayshiff

ayshiff commented Mar 25, 2021

Copy link
Copy Markdown
Author

@mitchellhamilton since creating those comments is done currently just after creating a release then this has to "spam" issues, creating a comment for each release from that single run. This definitely needs improvement and we should aggregate stuff to create at most one comment per issue.

I will try to aggregate the different issues to avoid duplicates and provide more complete comments.
Probably something like this:

// Inside createReleaseComments()
return [...new Set([...pulls, ...issues].map(({ number }) => number))].map(
  (number) => ({
      ...repo,
      issue_number: number,
      body: getReleaseMessage(htmlUrl, tagName),
    })
)

// In runPublish()
// Aggregate the differents comments
    const comments = await Promise.all(
      releasedPackages.map((pkg) =>
        createRelease(octokit, {
          pkg,
          tagName: `${pkg.packageJson.name}@${pkg.packageJson.version}`,
          comment,
        })
      )
    ).then((values) => 
       // Combine by issue number
    )
)

We will probably have something like this to build the body of the comments:

{
  "<ISSUE_NUMBER>": [
    {
      "htmlUrl": "<RELEASE_HTML_URL>",
      "tagName": "<RELEASE_TAG_NAME>",
    }
  ]
}

Tell me if this makes sense to you.

@Andarist

Copy link
Copy Markdown
Member

@ayshiff could you take a look at this comment? I think that answering those questions should be done first as those are high-level questions

@ayshiff

ayshiff commented Apr 3, 2021

Copy link
Copy Markdown
Author

I tried to answer the different requests in my last commit. (It's still a work in progress)

I added a condition to create comments only when we have a changeset.
I also make sure that there is only one comment per issue and that we can aggregate some information in this comment.

I currently display the entire list of changeset summaries, but I'll try to find a way to display only the information that is relevant to that comment.

I'm a bit busy at the moment but I'll try to find some time to test and improve this.

@Andarist

Andarist commented Apr 3, 2021

Copy link
Copy Markdown
Member

I'm a bit busy at the moment but I'll try to find some time to test and improve this.

No problem - I'm a little bit busy right now as well so I might not have time to review this right away. Please ping me when you want me to do the review :)

@ayshiff
ayshiff force-pushed the feature/release-comment branch from 2a85741 to 91daffb Compare April 22, 2021 07:41
@orta

orta commented Jul 9, 2021

Copy link
Copy Markdown
Contributor

( if you want an additional reference, I have built something like this before: https://github.com/danger/peril-settings/blob/master/org/new_tag.ts )

@acao

acao commented Jun 11, 2023

Copy link
Copy Markdown

@ayshiff looking at your last commits, I'm wondering, is this ready? what else is left?

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.

[feature] Comment on issues when packages are released

5 participants