-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore: Move release relevant actions to GH actions #4993
Conversation
…-changelogs # Conflicts: # yarn.lock
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.
Overall, it looks like a huge improvement; I just have a few minor comments.
@@ -13,7 +16,7 @@ export const validMessageTypes = [ | |||
interface Change { | |||
packageNames: string[]; | |||
commit: string; | |||
type: typeof validMessageTypes[number]; | |||
type: (typeof validMessageTypes)[number]; |
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.
[q] Unrelated to this PR and also not often used I guess, but do you know why 'Updated Dependencies' is already plural?
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 have no idea. But I am already working on a follow up where this won't matter anymore. I think this is not meant to be used manually.
Co-authored-by: Deeksha Sinha <[email protected]>
Co-authored-by: Deeksha Sinha <[email protected]>
const content = await readFile(fileName, { encoding: 'utf-8' }); | ||
const match = content.match(parseCs); | ||
if (!match) { | ||
console.error( |
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 did not find any replacement for this logic in your code.
I take it you deliberately removed this validation step?
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.
The validate changeset message types was moved to the check-pr/merge-changelog action, but maybe validate structure (parseCs
) should also be moved there?
const unifiedChangelog = await readFile('CHANGELOG.md', { encoding: 'utf8' }); | ||
await writeFile( | ||
'CHANGELOG.md', | ||
unifiedChangelog.split('\n').slice(0, 30).join('\n') + |
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.
Can you explain this a bit?
This looks like the first (arbitrarily?) 30 lines are taken, put in front of the new changelog & version, and then the remaining lines are attached at the end??
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 can't really to be honest. I think this just takes out the comments that we have in the changelog file. Nobody is touching it manually anyways, so they are not helpful. I didn't touch this logic in this PR, only moved it to a different place.
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.
LGTM, many nice refactors, before you merge though, make sure to read & answer my remaining comments.
There is quite some logic going on when merging the changesets, so I didn't want to repeat it. Also bumping and tagging the version is the same action, so moved all three of those into actions, so that we can reuse them in our other project.
I had to refactor a few places to not duplicate the code and I changes smaller things along the way (removing
unixEOL
and executing file write and read options async instead of sync).I left a few TODOs for potential follow ups (does not have to happen immediately, but I would like to attempt it on the friday ;) ). I would like to separate those from this already big PR.