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

armstorageimportexport - clean up client constructors #21522

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

jhendrixMSFT
Copy link
Member

We don't allow optional client constructor parameters for ARM. Use an autorest transform until the OpenAPI has been fixed.

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

We don't allow optional client constructor parameters for ARM.
Use an autorest transform until the OpenAPI has been fixed.
@github-actions github-actions bot added the Mgmt This issue is related to a management-plane library. label Sep 8, 2023
Copy link
Member

@tadelesh tadelesh left a comment

Choose a reason for hiding this comment

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

@jhendrixMSFT do you mean it is illegal to have optional client param for arm? There is another service with required client param:

func NewServicesClient(subscriptionID string, serviceName string, credential azcore.TokenCredential, options *arm.ClientOptions) (*ServicesClient, error) {
, it is legal?

@jhendrixMSFT
Copy link
Member Author

jhendrixMSFT commented Sep 11, 2023

The *arm.ClientOptions type is legal and expected. In your example, serviceName looks to be required?

Recent versions of the code generator will reject specs for ARM that contain optional client parameters.

@jhendrixMSFT jhendrixMSFT merged commit e63e5a1 into Azure:main Sep 11, 2023
11 checks passed
@jhendrixMSFT jhendrixMSFT deleted the armstorageimportexport-cleanup branch September 11, 2023 17:10
@jhendrixMSFT
Copy link
Member Author

I wonder though, should serviceName actually be a client parameter? It's almost always the wrong choice.

@tadelesh
Copy link
Member

I wonder though, should serviceName actually be a client parameter? It's almost always the wrong choice.

I'm also has such concern. Go just followed other languages. I'll try to confirm with service team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants