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

fix(spring): use newHttpJsonBuilder in settings bean for REST transport option #1227

Merged
merged 6 commits into from
Jan 10, 2023

Conversation

emmileaf
Copy link
Contributor

@emmileaf emmileaf commented Jan 6, 2023

This PR is a follow-up to GoogleCloudPlatform/spring-cloud-gcp#1364 (comment).

Updated behaviour (see #1078) for when Transport.REST_GRPC and useRest = true (through properties):

  • The ServiceSettings bean uses newHttpJsonBuilder
  • The TransportChannelProvider bean definition returns defaultHttpJsonTransportProviderBuilder

@emmileaf emmileaf added the spring pr that's related to spring code gen, intend to merge into autoconfig-gen-draft2 branch. label Jan 6, 2023
@emmileaf emmileaf marked this pull request as ready for review January 6, 2023 21:54
@emmileaf emmileaf requested a review from a team as a code owner January 6, 2023 21:54
.setReturnType(STATIC_TYPES.get("InstantiatingHttpJsonChannelProvider"))
.build();

if (hasRestOption) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is fine for preview release, but note that we need to come up with a strategy for REST only client like compute. Do we expect customers to always set this option to true or we automatically set this to true for REST only client? @zhumin8 @emmileaf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For post-preview, I think it would make sense for this property to be automatically set to true for REST-only clients.

Perhaps similar to the way we currently have GRPC-only clients default to useRest = True behaviour and do not provide the useRest property as an option to override, we can have REST-only clients default to useRest = True behaviour and also do not expose the useRest property to override?

Copy link
Contributor

@zhumin8 zhumin8 Jan 10, 2023

Choose a reason for hiding this comment

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

I agree with @emmileaf. I believe we are currently only populating useRest property in case Transport.GRPC_REST. So for REST only client, we could skip populating this property and set default to useRest = True behavior.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@emmileaf emmileaf merged commit 8736c61 into autoconfig-gen-draft2 Jan 10, 2023
@emmileaf emmileaf deleted the spring-use-rest-patch branch January 10, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spring pr that's related to spring code gen, intend to merge into autoconfig-gen-draft2 branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants