-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Add managementApiUrl option #45
Conversation
@@ -102,6 +105,7 @@ const run = async () => { | |||
|
|||
await generateModelsAsync({ | |||
environmentId: environmentId, | |||
managementApiUrl: resolvedArgs.managementApiUrl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the other parameters are in variables why not this one? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I don't think there is any reasonable value in doing so. If anything it obscures the data flow a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there is no real reason:) You could've unified it though because consistency is also important.
@@ -38,6 +38,7 @@ export interface IGenerateModelsConfig { | |||
isEnterpriseSubscription: boolean; | |||
sdkType: SdkType; | |||
apiKey: string; | |||
managementApiUrl?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the information about the URL to the readme. maybe here?: https://github.com/JiriLojda/model-generator-js/tree/master#cli-configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally left it, because I consider this an internal configuration key that most users outside Kontent.ai employees won't need. That being said there isn't much harm in adding it either.
Motivation
Sometimes it is useful (or even necessary) to specify a custom Kontent.ai Management API URL. (e.g. when using a proxy, or QA environment). This PR introduces a configuration option (in code only) that makes this possible.
Internal link: DEVREL-955