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

fix(javascript): release process #1779

Merged
merged 8 commits into from
Jul 19, 2023
Merged

fix(javascript): release process #1779

merged 8 commits into from
Jul 19, 2023

Conversation

shortcuts
Copy link
Member

🧭 What and Why

🎟 JIRA Ticket: -

Changes included:

fixes #1778

somehow the release process of the JavaScript client is broken (see #1778), it made me think that we could just leverage lerna for independant version bumping and remove any kind of JS-specific logic in the release bumping script.

to properly test this change, we can run yarn docker:release locally to see every packages getting bumped, then run the generation and cts generation to assert it works properly.


detailed changes

  • generators
    • remove js specific stuff in the config files
    • get package version from the package.json
    • get package name and path to package via the output field of the clients/openapitools.json file
  • scripts
    • treat javascript packages as the other packages
    • move updateDartPackages call to the updateAPIVersions method
    • add dart pub get to prevent dart cache issue on release

@shortcuts shortcuts requested a review from a team as a code owner July 18, 2023 22:12
@shortcuts shortcuts self-assigned this Jul 18, 2023
@shortcuts shortcuts requested review from damcou and millotp July 18, 2023 22:12
@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit 8dd9f02
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/64b7b57223199f0008a316e4
😎 Deploy Preview https://deploy-preview-1779--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Jul 18, 2023

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

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.

looks good ! love the logic removal

@@ -15,7 +15,7 @@
"folder": "clients/algoliasearch-client-javascript",
"npmNamespace": "@algolia",
"gitRepoId": "algoliasearch-client-javascript",
"utilsPackageVersion": "5.0.0-alpha.73",
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure you want to rename that ? I think utilsPackageVersion was fine, especially since we use this variable name elsewhere still

Copy link
Member Author

Choose a reason for hiding this comment

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

bah we were populating this version with the search client version, we know they will always be in sync so.. I thought it was easier

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe rename it to searchPackageVersion ? just to differentiate from ingestion and the other

Copy link
Member Author

Choose a reason for hiding this comment

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

or wdyt if I go with version so it's broader and cover many cases?

for JS it only concerns the utils package, but for the other clients it concerns the package itself

Copy link
Collaborator

Choose a reason for hiding this comment

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

version is fine, js will always have edge cases anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -36,53 +36,3 @@ After a full CI run, a release commit will be sent to every repository and sprea

Each language repository should have their own release process, and should run only when the latest commit starts with `chore: release`. By doing so, we have a way to just update the repository, for example READMEs, without having to release.

## Releasing manually
Copy link
Member Author

Choose a reason for hiding this comment

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

we are going stable soon and there's many way to release, even from the CI, we don't have to communicate how to do that and shouldn't recommend it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could be nice to keep this as internal doc somewhere

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.

small comments

@@ -36,53 +36,3 @@ After a full CI run, a release commit will be sent to every repository and sprea

Each language repository should have their own release process, and should run only when the latest commit starts with `chore: release`. By doing so, we have a way to just update the repository, for example READMEs, without having to release.

## Releasing manually
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could be nice to keep this as internal doc somewhere

@shortcuts
Copy link
Member Author

ah Go is broken brb

@shortcuts
Copy link
Member Author

shortcuts commented Jul 19, 2023

ok actually we've named it packageVersion and not version because it already contains the API version 🤷🏻

@shortcuts shortcuts requested a review from millotp July 19, 2023 10:20
@shortcuts
Copy link
Member Author

ok 0 changes now it's perfect, time to try a release and see if it works

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.

perfect ! sorry for the back and forth

@shortcuts shortcuts merged commit d97511a into main Jul 19, 2023
24 checks passed
@shortcuts shortcuts deleted the fix/javascript-release branch July 19, 2023 10:22
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.

None yet

3 participants