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.

Sign up for GitHub

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

Jump to bottom

refine factory CTOR logic #1496

Merged
merged 12 commits into from
Feb 21, 2025
Merged

refine factory CTOR logic #1496

merged 12 commits into from
Feb 21, 2025

Conversation

JiaqiZhang-Dev
Copy link
Member

resolve: #1488

@JiaqiZhang-Dev JiaqiZhang-Dev marked this pull request as ready for review February 17, 2025 01:19
tadelesh
tadelesh reviewed Feb 17, 2025
View reviewed changes
@tadelesh
Copy link
Member

@jhendrixMSFT could you also help to review?

@JiaqiZhang-Dev
Copy link
Member Author

JiaqiZhang-Dev commented Feb 20, 2025 •
edited
Loading

@jhendrixMSFT @tadelesh I have done a full tests for all current arm packages with the default config for factoryGatherAllParams which means client factory only gathers common parameters of client. There are about 20%(50/247) packages has a breaking change caused by NewClientFactory, such as armnetwork, armbilling, armfrontdoor and so on.

In this month release, there are two services have the breaking change in NewClientFactory function which bring a new parameter(quota, hybridconnectivity), so that we need to solve this problem asap.

@tadelesh
Copy link
Member

@jhendrixMSFT @tadelesh I have done a full tests for all current arm packages with the default config for factoryGatherAllParams which means client factory only gathers common parameters of client. There are about 20%(46/244) packages has a breaking change caused by NewClientFactory, such as armnetwork, armbilling, armfrontdoor and so on.

In this month release, there are two services have the breaking change in NewClientFactory function which bring a new parameter(quota, hybridconnectivity), so that we need to solve this problem asap.

are they generated from typespec or swagger? can we add the config based on your test which could prevent future breaking when releasing.

@JiaqiZhang-Dev
Copy link
Member Author

@jhendrixMSFT @tadelesh I have done a full tests for all current arm packages with the default config for factoryGatherAllParams which means client factory only gathers common parameters of client. There are about 20%(46/244) packages has a breaking change caused by NewClientFactory, such as armnetwork, armbilling, armfrontdoor and so on.
In this month release, there are two services have the breaking change in NewClientFactory function which bring a new parameter(quota, hybridconnectivity), so that we need to solve this problem asap.

are they generated from typespec or swagger? can we add the config based on your test which could prevent future breaking when releasing.

By swagger. Good suggestion! I'll add the config for those services.

@JiaqiZhang-Dev JiaqiZhang-Dev mentioned this pull request Feb 21, 2025
Open
5 tasks
@jhendrixMSFT
Copy link
Member

Can you please update the changelog with the info about this change? Also, please add an entry to the readme about the new command line switch.

@JiaqiZhang-Dev JiaqiZhang-Dev force-pushed the refine_client_factory_ctor_logic branch from 1b0a007 to a7778ad Compare February 21, 2025 15:32
@JiaqiZhang-Dev
Copy link
Member Author

Can you please update the changelog with the info about this change? Also, please add an entry to the readme about the new command line switch.

done~

@jhendrixMSFT jhendrixMSFT force-pushed the refine_client_factory_ctor_logic branch from a7778ad to 3a688e9 Compare February 21, 2025 16:21
@jhendrixMSFT jhendrixMSFT merged commit 4ee31e1 into main Feb 21, 2025
8 checks passed
@jhendrixMSFT jhendrixMSFT deleted the refine_client_factory_ctor_logic branch February 21, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tadelesh tadelesh tadelesh approved these changes

@jhendrixMSFT jhendrixMSFT jhendrixMSFT approved these changes

@chlowell chlowell Awaiting requested review from chlowell chlowell is a code owner

@RickWinter RickWinter Awaiting requested review from RickWinter RickWinter is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refine client factory CTOR logic to prevent potential breaking change
3 participants
@JiaqiZhang-Dev @tadelesh @jhendrixMSFT