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): manage utils package version to bump versions correctly #346

Merged
merged 7 commits into from
Apr 7, 2022

Conversation

eunjae-lee
Copy link
Contributor

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

🧭 What and Why

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

Changes included:

Solving the same problem as #340, but doing it differently, following the suggestion at #340 (comment)

@netlify
Copy link

netlify bot commented Apr 7, 2022

Deploy Preview for api-clients-automation failed.

Name Link
🔨 Latest commit 98b64d1
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/624f02b30dba7b000882fb71

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 7, 2022

✗ The generated branch has been deleted.

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

);
}
additionalProperties.packageVersion = newVersion;

if (additionalProperties.utilsPackageVersion) {
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 initialize/update it anyway, so that people don't have to add it themselves, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're saying we should just add it if it doesn't exist there but the generator is javascript-?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, since those packages are mandatory to properly use the client and they will all have the same version, we can assume that it should always be there

Copy link
Member

Choose a reason for hiding this comment

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

We could even assume that if it's not here, it should have the same value as javascript-search for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

762c373
good idea

Comment on lines +131 to +133
if (lang === 'javascript' && nextUtilsPackageVersion) {
additionalProperties.utilsPackageVersion = nextUtilsPackageVersion;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (lang === 'javascript' && nextUtilsPackageVersion) {
additionalProperties.utilsPackageVersion = nextUtilsPackageVersion;
}
if (lang === 'javascript') {
additionalProperties.utilsPackageVersion = nextUtilsPackageVersion || mainPackageUtilsVersion;
}

mainPackageUtilsVersion is openapitools['generator-cli'].generators[MAIN_PACKAGE.javascript] .additionalProperties.utilsPackageVersion

So it's initialized in case it's missing anyway? wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's worth handling it actually as the gen would throw if there's no utilsPackageVersion 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case we're not releasing javascript package, we shouldn't bump the utils version in the openapitools.json. I'll add the comment.

shortcuts
shortcuts previously approved these changes Apr 7, 2022
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.

Looks bon!

@eunjae-lee eunjae-lee enabled auto-merge (squash) April 7, 2022 15:33
@eunjae-lee eunjae-lee merged commit edea9a0 into main Apr 7, 2022
@eunjae-lee eunjae-lee deleted the chore/js-versioning-2 branch April 7, 2022 15:39
algolia-bot added a commit to algolia/algoliasearch-client-javascript that referenced this pull request Apr 7, 2022
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
)

* chore(ci): manage utils package version to bump versions correctly

* chore: add utils version if not exist

* chore: add comment
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