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

fix: add default library settings for incorrect lib version #2212

Merged
merged 5 commits into from
Oct 10, 2024
Merged

fix: add default library settings for incorrect lib version #2212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac460c2
fix: add default library settings for incorrect lib version
ohmayr Oct 9, 2024
42efd9f
minor fix
ohmayr Oct 10, 2024
2bb3873
fix style
ohmayr Oct 10, 2024
f643db2
add unit test
ohmayr Oct 10, 2024
bb3b20c
fix style
ohmayr Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions gapic/schema/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,13 +706,14 @@ def all_library_settings(
for library_setting in self.service_yaml_config.publishing.library_settings
}

# Add default settings for the current proto package
if not result:
result = {
self.naming.proto_package: client_pb2.ClientLibrarySettings(
version=self.naming.proto_package
# NOTE: Add default settings for the current proto package
# for the following cases:
# - if library settings are not specified in the service config.
# - if library_settings.version != self.naming.proto_package (proto package name)
if self.naming.proto_package not in result:
result[self.naming.proto_package] = client_pb2.ClientLibrarySettings(
version=self.naming.proto_package
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, you're not passing python_settings here like you are in line 704 because we intend to pass empty settings since they weren't explicitly specified for self.naming.proto_package version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to set empty settings against self.naming.proto_package if the key is not already set in line 704 which happens only when someone has an incorrect library_settings.version configured in their service yaml. We're doing this so our generator doesn't if it fails to find the settings that it's looking for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sg. You're confirming the intent I phrased, so I understood correctly. Thanks!

)
Comment on lines +714 to 716
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
result[self.naming.proto_package] = client_pb2.ClientLibrarySettings(
version=self.naming.proto_package
)
result[self.naming.proto_package] = client_pb2.ClientLibrarySettings(
version=self.naming.proto_package
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this results in the style check to fail.

}

return result

Expand Down
29 changes: 29 additions & 0 deletions tests/unit/schema/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2741,6 +2741,35 @@ def test_read_empty_python_settings_from_service_yaml():
== client_pb2.PythonSettings.ExperimentalFeatures()
assert api_schema.all_library_settings["google.example.v1beta1"].python_settings.experimental_features.rest_async_io_enabled \
== False
assert api_schema.all_library_settings[api_schema.naming.proto_package].python_settings \
== client_pb2.PythonSettings()


def test_incorrect_library_settings_version():
# NOTE: This test case ensures that the generator is able to read
# from the default library settings if the version specified against the
# library settings in the service yaml of an API differs from the version
# of the API.
service_yaml_config = {
"apis": [
{"name": "google.example.v1beta1.ServiceOne.Example1"},
],
"publishing": {
"library_settings": [
{
"version": "google.example.v1",
"python_settings": {
"experimental_features": {"rest_async_io_enabled": True},
},
}
]
},
}
cli_options = Options(service_yaml_config=service_yaml_config)
fd = get_file_descriptor_proto_for_tests(fields=[])
api_schema = api.API.build(fd, "google.example.v1beta1", opts=cli_options)
assert api_schema.all_library_settings[api_schema.naming.proto_package].python_settings \
== client_pb2.PythonSettings()


def test_python_settings_duplicate_version_raises_error():
Expand Down