fix(generator): add hostWithFallback
#479
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
shortcuts
requested review from
a team,
eunjae-lee and
millotp
and removed request for
a team
|
oops not good for now :D edit: good now |
| @@ -38,7 +38,7 @@ export async function generate( | |||
|
|
|||
| await generateOpenapitools(generators); | |||
|
|
|||
| const availableWorkspaces = await run('yarn workspaces list', { verbose }); | |||
| const availableWorkspaces = await run('yarn workspaces list'); | |||
There was a problem hiding this comment.
I forgot to push this fix last time
| @@ -50,11 +50,11 @@ public class {{classname}} extends ApiClient { | |||
|
|
|||
| {{^hasRegionalHost}} | |||
| public {{classname}}(String appId, String apiKey) { | |||
| this(appId, apiKey, new HttpRequester(getDefaultHosts({{^isExperimentalHost}}appId{{/isExperimentalHost}})), null); | |||
There was a problem hiding this comment.
Where is this variable isExperimentalHost created ? Is there any reason why it's used in the java and javascript templates and not in the php one ? Did I miss something ?
There was a problem hiding this comment.
You missed it because you did not generated the Predict client for PHP
It was there because the URL was not following the correct schema, but in fact the ew value was a region.
We now don't use this variable anymore
| @@ -102,7 +102,8 @@ use {{invokerPackage}}\RetryStrategy\ClusterHosts; | |||
| // If a list of hosts was passed, we ignore the cache | |||
| $clusterHosts = ClusterHosts::create($hosts); | |||
| } else { | |||
| $clusterHosts = ClusterHosts::create('{{host}}.'.$config->getRegion().'.algolia.{{topLevelDomain}}'); | |||
| $url = str_replace('{region}', $config->getRegion(), '{{{host}}}'); | |||
There was a problem hiding this comment.
What does the {{{host}}} variable is supposed to contain now ? Has it changed ? Before it was "analytics", "insights", ... no ?
There was a problem hiding this comment.
Yep, but it contained a lot of logic like topLevelDomain etc.
It stores the whole domain now, and we only replace the region accordingly, so we don't have to compute other stuff than host
For insights for example its value is insights.{region}.algolia.io, so we only replace the {region} field with the region value
There was a problem hiding this comment.
You can see it in action here btw: dd5d4db
to make sure the logic is right for PHP
Only JavaScript have the client tests |
| } | ||
| if (fallbackToAliasHost) { |
There was a problem hiding this comment.
you don't need the if here, we won't read it anyway if fallbacktoAliasHost is false.
There was a problem hiding this comment.
Good catch! Done d7688cf
| @@ -10,7 +10,12 @@ components: | |||
| apiKey: | |||
| $ref: '../common/securitySchemes.yml#/apiKey' | |||
| servers: | |||
| - url: https://predict-api-432xa6wemq-ew.a.run.app | |||
| - url: https://predict-api-432xa6wemq-{region}.a.run.app | |||
There was a problem hiding this comment.
You should switch the order of the stack PR so this doesn't appear here.
There was a problem hiding this comment.
It's not the same change as in #473, it just fixes the spec because ew is in fact a region
There was a problem hiding this comment.
ah ok I didn't know ! Which region is it ?
There was a problem hiding this comment.
Western Europe I believe? cc @anghelalexandra :D
There was a problem hiding this comment.
Ok, I created a ticket about it for PHP https://algolia.atlassian.net/browse/APIC-466 |
Thanks! We indeed need one for Java too |
|
🧭 What and Why
🎟 JIRA Ticket: -
Changes included:
Follow up of #471
Adds a new
hostWithFallbackvariable to avoid having to compute the domain etc.It also allow path with other format to work (see #473)
🧪 Test
CI :D