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(ci): update js version bumping strategy #340

Closed
wants to merge 6 commits into from

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Apr 5, 2022

🧭 What and Why

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

Changes included:

Because of the limitation of templating, it’s hard to update all the versions of packages and their dependencies correctly. What we’ve been doing is,

  • If it’s a generated package, its version will be updated by the new version in openapitools.json
  • If it’s not a generated package (algoliasearch, -common, requesters), we need to manually bump their versions.

Not only that, but also we need to update all the dependencies of those packages. And because we heavily rely on templating, it’s hard to do such complicated task.

So we’re updating version bumping strategy a bit. My proposal here is to split the generation part for the js from the others. I've left comments in the code. Let me know if you need more clarification.


For example, this was trying to release 0.0.6, but the expression ^0.0.5 made it resolve to 0.0.5, instead of 0.0.6, which caused the build to fail.
https://github.com/algolia/algoliasearch-client-javascript/runs/5833650231?check_suite_focus=true

@netlify
Copy link

netlify bot commented Apr 5, 2022

Deploy Preview for api-clients-automation failed.

Name Link
🔨 Latest commit 8c9fa6f
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/624daed27bd1df0007a68740

@eunjae-lee eunjae-lee requested review from a team, damcou and shortcuts and removed request for a team April 5, 2022 16:10
@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 5, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the generated/main branch.

// We generate javascript packages BEFORE updating the versions in `openapitools.json`,
// because we bump the versions with lerna after the generation.
// The reason behind this is that it's hard to update the versions of packages
// and the dependencies correctly, due to the limitation of templating.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the limitation of templating be overcome with the custom generator ? If it about passing an object with all version to the template it's doable, unless it's easier to do it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be precise, we need the new versions of client-common, and requesters packages, and pass them to each client (client-analytics, client-...), so that those dependencies will have correct version. I haven't touched the custom generator, but I believe it's doable. However, I didn't explore that path, because it involves too much "custom" handling. (This PR introduces another type of "custom" code, though 😅)

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I don't know which one is best, we need to keep all the custom code in one place and not spread a lot but it's already done ahah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but the reason why I didn't explore that custom generator is, because bumping version can be really tricky. I already did the work previously in this repo, but it didn't work due to edge cases like this. And lerna version is actually a good tool to take care of everything out of the box. So the direction in this PR is to maximise the benefit of lerna version instead of doing much custom thing here.

Copy link
Member

Choose a reason for hiding this comment

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

What if the JavaScript openapitools object include a new variable for the dependency version, that we can use in the template? So the updateOpenApiTools update it with the correct bump and we can then generate without making exceptions

Copy link
Member

Choose a reason for hiding this comment

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

it indeed adds a new variable, but it's fully controlled by the script and have the minimal footprint

Copy link
Contributor Author

@eunjae-lee eunjae-lee Apr 6, 2022

Choose a reason for hiding this comment

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

@shortcuts That's an idea. It means we need to add something like

      "javascript-search": {
        "generatorName": "algolia-javascript",
        "templateDir": "#{cwd}/templates/javascript/",
        "config": "#{cwd}/openapitools.json",
        "apiPackage": "src",
        "output": "#{cwd}/clients/algoliasearch-client-javascript/packages/client-search",
        "glob": "specs/bundled/search.yml",
        "gitHost": "algolia",
        "gitUserId": "algolia",
        "gitRepoId": "algoliasearch-client-javascript",
        "reservedWordsMappings": "queryParameters=queryParameters,requestOptions=requestOptions",
        "additionalProperties": {
          "modelPropertyNaming": "original",
          "supportsES6": true,
          "npmName": "@experimental-api-clients-automation/client-search",
          "buildFile": "client-search",
          "apiName": "search",
          "capitalizedApiName": "Search",
          "packageVersion": "0.0.5",
          "packageName": "@experimental-api-clients-automation/client-search"
+         "clientCommonPackageVersion": "0.0.5",
+         "clientRequesterNodePackageVersion": "0.0.5",
+         "clientRequesterBrowserPackageVersion": "0.0.5"
        }
      },

to every javascript-xxx generator. Then, we'll be able to use those variables in package.mustache.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Since those are internal package it's fine to sync their versions and only have one value IMO, this way we could even introduce new ones without changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this suggestion, I opened a new PR #346 and it seems much cleaner! Thanks. Closing this PR.

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.

4 participants