Skip to content

chore(scripts): factor git repo id #499

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

Merged
merged 2 commits into from
May 13, 2022
Merged

chore(scripts): factor git repo id #499

merged 2 commits into from
May 13, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented May 12, 2022

🧭 What and Why

remove almost all guessable value from openapitools.json and put it in clients.config.json or generators (because it's defined per language).

This doesn't change much, just simplify some logic.

🧪 Test

CI

@millotp millotp self-assigned this May 12, 2022
@netlify
Copy link

netlify bot commented May 12, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 53c62e1
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/627e0f240f242a0008a2c0af

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 12, 2022

✗ The generated branch has been deleted.

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

@@ -5,7 +5,6 @@
"javascript-search": {
Copy link
Member

Choose a reason for hiding this comment

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

I believe there are others in this file that we could also factor like apiPackage for JS etc.

Copy link
Member

Choose a reason for hiding this comment

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

I found those:

js:

  • apiPackage

java:

  • groupId
  • apiPackage
  • invokerPackage
  • library

php:

  • I can only see some leftovers for php-query-suggestions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the list ! I removed all of it

@millotp millotp marked this pull request as draft May 12, 2022 11:29
@millotp millotp force-pushed the chore/common-gitrepoid branch from 925c6b0 to 02b74a0 Compare May 12, 2022 14:09
@millotp millotp force-pushed the fix/java-release-name branch 2 times, most recently from 36b225f to ae4d23b Compare May 12, 2022 15:25
@millotp millotp force-pushed the chore/common-gitrepoid branch from 02b74a0 to 422e8d0 Compare May 12, 2022 15:50
@millotp millotp marked this pull request as ready for review May 12, 2022 15:51
@millotp millotp requested review from a team, eunjae-lee, damcou and shortcuts and removed request for a team and eunjae-lee May 12, 2022 15:51
Base automatically changed from fix/java-release-name to main May 12, 2022 16:33
@millotp millotp force-pushed the chore/common-gitrepoid branch from 422e8d0 to 5b80150 Compare May 12, 2022 16:34
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Good improvement, I just had two small questions but nothing bad. 🚀

@@ -72,6 +72,7 @@ public void processOpts() {
// generator specific options
setApiNameSuffix(Utils.API_SUFFIX);
setParameterNamingConvention("camelCase");
additionalProperties.put("invokerPackage", "Algolia\\AlgoliaSearch");
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't possible to move it in clients.config.json instead ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it could be the purpose of clients.config.json is to make data available to everything: scripts, generators, ci...
here it's only needed in the generators so I put it here

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I thought the main goal was just to deduplicate the config, but whatever

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the openapitools.json is fed directly into the generators, it is for deduplicating config so we can put it in the common place, that is generators

@@ -88,4 +89,8 @@ public void processOpts() {
)
);
}

public String getComposerPackageName() {
return "algolia/algoliasearch-client-php";
Copy link
Contributor

Choose a reason for hiding this comment

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

The default method which computes the name doesn't work anymore without gitRepoId in the openapitool.json file ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I don't really get why, there is a bug in openapi generator where if no value is provided for gitRepoId, it will default to GIT_REPO_ID (stupid code) and break the getComposePackageName function.
But this does the same in the end

@millotp millotp merged commit f903ebd into main May 13, 2022
@millotp millotp deleted the chore/common-gitrepoid branch May 13, 2022 09:03
@millotp
Copy link
Collaborator Author

millotp commented May 13, 2022

Thanks for the review :)

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