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

Add api version in ClientOptions and policy inside generated client #2561

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Jun 7, 2024

fixes Azure/azure-sdk-for-js#29883 and #2396 (comment) in favor of Azure/azure-sdk-for-js#29904

if there's no client level api version, we will:

  1. not generate apiVersion in options
    which means we will not generate a property of apiVersion in the ServiceClientOptions interface.
  2. remove ApiVersionPolicy
    which means, we will generate code client.pipeline.removePolicy({ name: "ApiVersionPolicy" }); after createClient from core-client-rest.
  3. add log warning
    if there's no client level api version, we will add a warning
    logger.warning("This client does not support client api-version, please change it at the operation level");
    if there's client level api version, but it's required, and it doesn't have a default value, apiVersion will be a positional argument for client factory function. we will ignore api version passed in options, and add a warning
    logger.warning("This client does not support to set api-version in options, please change it at positional argument");
    ApiVersionPolicy will also be removed in this case
  4. not add client side ClientApiVersionPolicy
    if there's no client level api version, we don't need to add this policy here, but if there's client level api version in the query position, according to [Deprecate]: deprecate apiVersion in ClientOptions from core-client-rest azure-sdk-for-js#29883 (comment) we will generate a new ClientApiVersionPolicy in the client side, so that we can remove the ApiVersionPolicy from core-client-rest in the future.
  hasDefault: true  required: true hasDefault: true required: false hasDefault: false required: true hasDefault: false required: false
Path Required: false
Generate ApiVersion in Options: true
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: false
{ apiVersion = "default", …options }
Required: false
Generate ApiVersion in Options: true
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: false
{ apiVersion = "default", …options }
Required: true
Generate ApiVersion in Options: false
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: false
Options
logWarning: true
Required: false
Generate apiVersion in options: true
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: false
{ apiVersion, …options }
Query Required: false
Generate ApiVersion in Options: true
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: true
{ apiVersion = "default", …options }
Required: false
Generate ApiVersion in Options: true
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: true
{ apiVersion = "default", …options }
Required: true
Generate ApiVersion in Options: false
Remove ApiVersionPolicy: true
Add clientApiVersionPolicy: true
Options
logWarning: true
Required: false
Generate apiVersion in options: true
Remove apiVersionPolicy: true
Add clientApiVersionPolicy: true
{ apiVersion, …options }

Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Looks great, left a comment on how to avoid that createClient causes side effects, should be easy to address. Please consider taking the suggestion

@qiaozha qiaozha marked this pull request as ready for review June 17, 2024 02:12
@qiaozha qiaozha changed the title Remove api version from options and add policy inside client Add api version inside ClientOptions and policy inside generated client Jun 17, 2024
@qiaozha qiaozha changed the title Add api version inside ClientOptions and policy inside generated client Add api version in ClientOptions and policy inside generated client Jun 17, 2024
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks good, just two minor remarks. 👍

@qiaozha qiaozha merged commit b0853b2 into Azure:main Jun 26, 2024
14 checks passed
@qiaozha qiaozha deleted the remove-api-version-from-options-and-add-policy-inside-client branch June 26, 2024 09:20
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.

[Deprecate]: deprecate apiVersion in ClientOptions from core-client-rest
4 participants