-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix repo links in updated changelogs to match auto-changelog
#165
base: main
Are you sure you want to change the base?
Conversation
auto-changelog
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
* @returns The HTTPS URL of the repository, e.g. | ||
* `https://github.com/OrganizationName/RepositoryName`. | ||
*/ | ||
export async function getRepositoryHttpsUrl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split up this function into two pieces so that it could be tested better: I extracted the code to get the repository URL to project.ts
and the string manipulation logic to misc-utils.ts
.
packageManifest: UnvalidatedPackageManifest, | ||
repositoryDirectoryPath: string, | ||
): Promise<string> { | ||
// Set automatically by NPM or Yarn 1.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although handling NPM or Yarn 1.x projects is not strictly necessary here, because create-release-branch
only supports monorepos, and all of our monorepos use Yarn Modern, I figured I'd include it here because I want to copy this function back to auto-changelog
after this PR is merged.
For context the original source for this logic is here: https://github.com/MetaMask/auto-changelog/blob/ef3e86e15b0de7061856a53fd18c4f38e898f5e8/src/repo.ts#L20
return convertToHttpsGitHubRepositoryUrl(packageManifest.repository.url); | ||
} | ||
|
||
const gitConfigUrl = await getStdoutFromCommand( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that most of our projects ought to have a repository
field in package.json
, I am not sure that this is really needed, but I figured I'd leave it here.
I tracked down the original source for this logic, and it comes from here: MetaMask/action-monorepo-release-pr@af8d750#diff-b2d6c9ae0590887f3d213032d057be61bd3e628650fb7ba99391bdc84587e454R25
* @returns The HTTPS URL of the repository, e.g. | ||
* `https://github.com/OrganizationName/RepositoryName`. | ||
*/ | ||
export function convertToHttpsGitHubRepositoryUrl(url: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is not in auto-changelog
, but is in this repo, so we're actually doing extra. I was curious why there was a distinction, so I did a bit of digging:
- In building
create-release-branch
, I took a lot of code fromaction-create-release-pr
- We used to have a separate repo called
action-monorepo-release-pr
, and that got merged intoaction-create-release-pr
- In particular this logic comes from here: MetaMask/action-monorepo-release-pr@af8d750#diff-b2d6c9ae0590887f3d213032d057be61bd3e628650fb7ba99391bdc84587e454R25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I've simplified the original logic to take a full-on regex approach. It seemed that the code wanted to go there.
When this tool populates the Unreleased section of the changelog, it does not just rewrite that section, but rather, it rewrites the entire changelog. That behavior comes from the `updateChangelog` function in `@metamask/auto-changelog`. This function takes the changes to apply, but it also takes a repository URL, which it will use to construct links for all of the releases in the changelog. Unfortunately, the repository URL that this tool passes to `updateChangelog` is not always the same URL that `auto-changelog` uses. Specifically, if the developer used a different URL to clone the repo than is listed in `package.json` under `repository.url`, then when creating a new release branch, all of the links in the changelog will be modified to reflect this new URL. This is incorrect, the resulting changelog will not pass validation when run through `auto-changelog validate`, forcing the developer to revert the changes to the links in order to merge the release PR. This commit addresses this pain point by updating the logic used to obtain the repository URL to better match what is in `@metamask/auto-changelog`. This means that the aforementioned links in the changelog should stay the same when creating a new release branch.
80b05fc
to
68f1121
Compare
@@ -77,13 +77,9 @@ describe('create-release-branch (functional)', () => { | |||
}, | |||
}); | |||
|
|||
expect(await environment.readJsonFile('package.json')).toStrictEqual({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are updated because there's now an extra repository
field in package.json
. That isn't really relevant to these tests, so I've simplified the assertions to only consider name
and version
.
When this tool populates the Unreleased section of the changelog, it does not just rewrite that section, but rather, it rewrites the entire changelog. That behavior comes from the
updateChangelog
function in@metamask/auto-changelog
. This function takes the changes to apply, but it also takes a repository URL, which it will use to construct links for all of the releases in the changelog.Unfortunately, the repository URL that this tool passes to
updateChangelog
is not always the same URL thatauto-changelog
uses under the hood. Specifically, if the developer used a different URL to clone the repo than is listed inpackage.json
underrepository.url
, then when creating a new release branch, all of the links in the changelog will be modified to reflect this new URL. This is incorrect, as the resulting changelog will not pass validation when run throughauto-changelog validate
, forcing the developer to revert the changes to the links in order to merge the release PR.This commit addresses this pain point by updating the logic used to obtain the repository URL to better match what is in
@metamask/auto-changelog
. This means that the aforementioned links in the changelog should stay the same when creating a new release branch.Fixes #72.