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.

Sign up for GitHub

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

Jump to bottom

use github api to find pull request for commit #46

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

use github api to find pull request for commit #46

wants to merge 1 commit into from

Conversation

patricklx
Copy link
Contributor

No description provided.

@patricklx patricklx force-pushed the get-pull-request-via-api branch from fb46ac6 to 47137ba Compare March 5, 2025 17:12
kategengler
kategengler reviewed Mar 5, 2025
View reviewed changes
src/changelog.ts
@@ -180,8 +176,11 @@ export default class Changelog {
await pMap(
commitInfos,
async (commitInfo: CommitInfo) => {
if (commitInfo.issueNumber) {
commitInfo.githubIssue = await this.github.getIssueData(this.config.repo, commitInfo.issueNumber);
const issueData = await this.github.getPullRequest(this.config.repo, commitInfo.commitSHA);
Copy link

Choose a reason for hiding this comment

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

I think the call to list-pull-requests may need a branch filter on it ... otherwise if working on another branch in the repo and PRing to it, this will return the first created PR that included a commit:

branch: next-feature
PR go to next-feature
PR next-feature to main
... release

Do you expect to see the PRs to next-feature in the CHANGELOG or the PR of next-feature to main?

Copy link
Contributor Author

@patricklx patricklx Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The commits are filtered with --first-parent. So we will only call the api for the merge commit or pushed commits directly to branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a problem if one passes --from --to params and one of them is not first parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved the code to handle duplicate PRs in case of custom from...to params

@patricklx patricklx force-pushed the get-pull-request-via-api branch 2 times, most recently from 4998a51 to 4ac024c Compare March 5, 2025 19:20
@patricklx patricklx marked this pull request as draft March 5, 2025 19:28
@patricklx patricklx force-pushed the get-pull-request-via-api branch 2 times, most recently from 5982878 to 15c82fa Compare March 5, 2025 21:07
@patricklx patricklx marked this pull request as ready for review March 5, 2025 21:07
@patricklx patricklx force-pushed the get-pull-request-via-api branch 2 times, most recently from 6ea65a6 to 819c5e2 Compare March 6, 2025 11:18
mansona
mansona requested changes Mar 6, 2025
View reviewed changes
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Almost there! there are a few things in these questions/suggestions that need to be fixed, not necessarily all of them 👍

src/__snapshots__/changelog.spec.js.snap
"commitSHA": "a0000003",
"date": "2017-01-01",
"githubIssue": {
Copy link
Member

Choose a reason for hiding this comment

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

why is this snapshot changing? 🤔 are we missing the new setup for the issuenumber in question in the test that is generating this snapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats because the PR is also in a0000004, and only 1 PR link will be set

src/changelog.spec.js
"https://api.github.com/repos/embroider-build/github-changelog/issues/2": {
body: {
const prCache = {
a0000001: [],
Copy link
Member

Choose a reason for hiding this comment

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

why are commit shas showing up in the PR cache here? 🤔

src/functional/markdown-full.spec.js
},
};
const issuesCache = {
"https://api.github.com/repos/embroider-build/github-changelog/issues/1": {
body: {
a0000001: [],
Copy link
Member

Choose a reason for hiding this comment

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

again I'm surprised there is anything in here 🤔 why can't we just leave it empty for that commit sha?

Copy link
Contributor Author

@patricklx patricklx Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is now a commit to PRs map. And it's a list of prs. Because api returns a list of prs

src/github-api.ts
}
return user.data;
} catch (e) {
if ((e as any).status === 404) {
Copy link
Member

Choose a reason for hiding this comment

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

so this is new functionality 🤔 it looks like the old behaviour was that it would throw an error here. Can you put it back to the old functionality here please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then it would fail locally if any new commit is added and then try release plan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot mimic the old behaviour. The old behaviour was to error out if the PR does not exist.
But now, we do assume that we do not know the linked PR number to a commit and use the API to find it.
A commit without a PR can be valid

@patricklx patricklx force-pushed the get-pull-request-via-api branch 4 times, most recently from 15600ba to 10c853b Compare March 6, 2025 12:07
patricklx
patricklx commented Mar 6, 2025
View reviewed changes
src/functional/markdown-full.spec.js
@@ -157,168 +162,160 @@ const listOfFileForEachCommit = {
a0000015: ["untitled/script.md"],
a0000016: ["packages/return-of-the-jedi/package.json"],
a0000017: ["packages/return-of-the-jedi/package.json"],
a0000018: ["packages/return-of-the-jedi/package.json"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this tests the case of a local commit for which no online commit can be founds

patricklx
patricklx commented Mar 6, 2025
View reviewed changes
src/functional/markdown-full.spec.js
{
sha: "a0000018",
refName: "",
summary: "local testing, not pushed",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here, local commit test

@patricklx patricklx force-pushed the get-pull-request-via-api branch 2 times, most recently from 9a81c81 to afb61ba Compare March 6, 2025 14:16
@patricklx patricklx marked this pull request as draft March 9, 2025 16:37
@patricklx patricklx force-pushed the get-pull-request-via-api branch from afb61ba to 9790c14 Compare March 10, 2025 16:37
@patricklx patricklx force-pushed the get-pull-request-via-api branch from 9790c14 to 7b14417 Compare March 10, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kategengler kategengler kategengler left review comments

@mansona mansona mansona requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patricklx @kategengler @mansona