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.

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: automate release #120

Merged
merged 50 commits into from
Feb 17, 2022
Merged

chore: automate release #120

merged 50 commits into from
Feb 17, 2022

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Feb 3, 2022

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-297

Changes included:

  • yarn release creates a release issue.
  • When the issue is approved and closed, scripts/release/process-release.js is run, and updates versions and changelogs.
ScreenShot.2022-02-03.at.16.18.31-converted.2.mp4

TODO in another PRs

  • yarn generate ${lang} is failing in process-release.js. Investigate.
  • Push generated clients into their own repositories. (Create new dummy repositories, and use them)
    • Try submodule
    • And, git clone + copy the generated + git commit + git push

Once done, we can prepare CI per repository.

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Feb 15, 2022

By the way, I will convert this to TS in a separate PR :)

@eunjae-lee eunjae-lee requested a review from shortcuts February 15, 2022 10:34
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Mostly implementation details comments, but this looks solid!

config.json Outdated
@@ -0,0 +1,8 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

If we put it at the root of the repository we could maybe have a more descriptive name, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we release this actual repo or just the one generated ?

Copy link
Member

Choose a reason for hiding this comment

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

We don't release it, it's just publicly available but we already have a few configs everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we put it at the root of the repository we could maybe have a more descriptive name, wdyt?

I actually wrapped it with the key "release" for the reason. What do you think? Should we have a separate "release.config.json"?

Copy link
Member

Choose a reason for hiding this comment

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

If you think of user use cases of this config, it's fine to keep it like that, otherwise yep it would make sense to have a release.config.json file only

Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Insane work !

config.json Outdated
@@ -0,0 +1,8 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we release this actual repo or just the one generated ?

].additionalProperties.packageVersion = versionsToRelease[lang].next;
}
});
fs.writeFileSync('openapitools.json', JSON.stringify(json, null, 2));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use fs/promises pls ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these scripts already use a bunch of synchronous code like execa.commandSync. If we go with fs/promises, we need to go all in, but I'm not sure how it's beneficial.

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

:kermittyping: congrats it looks really nice!!

config.json Outdated
@@ -0,0 +1,8 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

If you think of user use cases of this config, it's fine to keep it like that, otherwise yep it would make sense to have a release.config.json file only

return;
}

if (commits.some((commit) => commit.message.includes('BREAKING CHANGE'))) {
Copy link
Member

Choose a reason for hiding this comment

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

We should note somewhere that introducing a breaking change requires our commit to contain it then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually comes from https://www.conventionalcommits.org/
Do you think we need the mention somewhere in readme or contribution guide?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a small md file in the doc folder describing commits and semantic titles, but this could be done in a later task (let's add it to our backlog still)


dotenv.config();

function getMarkdownSection(markdown: string, title: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth having test for such scripts? I feel like the complexity requires it

Copy link
Member

Choose a reason for hiding this comment

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

(not needed in this PR of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have such code once or twice again, then we could setup jest, I guess. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yep I think it would help us especially if we encounter edge cases in the future

@eunjae-lee eunjae-lee requested a review from shortcuts February 17, 2022 10:11
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

I think it's good for a first iteration of this release process, congrats!

Could you please add those 2 tickets to our backlog?

  • Add doc about commits and semantic PR titles
  • Write basic tests for this release process

Don't hesitate to do more testing on this repository (even if it floods the issue), so we can see how it behaves in detail.

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.

3 participants