Skip to content
This repository has been archived by the owner on May 21, 2021. It is now read-only.

Add changelog updating #23

Merged
merged 6 commits into from
May 12, 2021
Merged

Add changelog updating #23

merged 6 commits into from
May 12, 2021

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented May 9, 2021

Adds changelog updating to the package update workflow via @metamask/auto-changelog. The action now assumes that there's a Keep-a-Changelog CHANGELOG.md file at the root of every package in the repository.

Base automatically changed from polyrepo-compatibility to main May 11, 2021 18:46
@rekmarks rekmarks force-pushed the changelog-updating branch from af9b7c6 to 56af347 Compare May 11, 2021 18:52
@rekmarks rekmarks requested a review from Gudahtt May 11, 2021 19:00
@rekmarks rekmarks marked this pull request as ready for review May 11, 2021 19:00
@rekmarks
Copy link
Member Author

rekmarks commented May 11, 2021

The package manifest types in this are improved relative to #22, but we still have to cast in one place. I'll make it actually type safe and sane in a follow-up after this or #24.

Edit: Done in #25.

{
files: ['**/*.d.ts'],
rules: {
'import/unambiguous': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

Why disable this? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Wait, on type definitions!? How does this rule apply to those 😕

Copy link
Member

Choose a reason for hiding this comment

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

Well, I've looked into this, but I am still confused. No update. Welp.

I'm OK to let this slide for now but I still suspect we're doing something wrong.

Copy link
Member Author

@rekmarks rekmarks May 12, 2021

Choose a reason for hiding this comment

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

I think it makes sense given the documentation of import/unambiguous: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/unambiguous.md

And indeed, using parserOptions: { sourceType: 'script' } } in the override works equally well. Maybe we prefer that?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure 🤔 I'm curious which part of the docs indicate that this makes sense 😅 That is, it's not clear to me that a type definition is a module or a script. I don't know which fits better, if either. I was hoping that the TypeScript ESLint module would make some sense from this, but no such luck.

Given my limited understanding at the moment, turning it off seems like the most sensible approach.

* the git config remote.origin.url string matches one of:
*
* - https://github.com/OrganizationName/RepositoryName
* - [email protected]:OrganizationName/RepositoryName.git
Copy link
Member

@Gudahtt Gudahtt May 12, 2021

Choose a reason for hiding this comment

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

Any idea when each of these are used? The @action/checkout docs indicate that github.repository is the default, but I haven't tracked down what that is yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I think it's simply dictated by whether git is configured to use SSH or HTTP/HTTPS. If you search for github.repository here, you'll see that it's just OrganizationName/RepositoryName.

actions/checkout supports either HTTPS or SSH, and the one it uses is determined by the inputs you provide to it, so for present purposes I think we're good so long as we support both?

Copy link
Member

Choose a reason for hiding this comment

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

actions/checkout supports either HTTPS or SSH, and the one it uses is determined by the inputs you provide to it

I couldn't find confirmation of this in their docs, but I found some code that indicates that this is true! https://github.com/actions/checkout/blob/main/src/url-helper.ts

const { newVersion, repositoryUrl } = updateSpecification;

let changelogContent: string;
const changelogPath = `${projectRootDirectory}/CHANGELOG.md`;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Probably better to use path.join here for Windows compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 19c3a07

*/
function getTruncatedPath(absolutePath: string): string {
return absolutePath
.split('/')
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we can use path.sep here for better windows support

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in b5096e0

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks merged commit af8d750 into main May 12, 2021
@rekmarks rekmarks deleted the changelog-updating branch May 12, 2021 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants