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

feat: populate .repo-metadata.json from highest version #2890

Merged
merged 6 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion library_generation/generate_composed_library.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def generate_composed_library(
base_arguments = __construct_tooling_arg(config=config)
owlbot_cli_source_folder = util.sh_util("mktemp -d")
os.makedirs(f"{library_path}", exist_ok=True)
for gapic in library.gapic_configs:
for gapic in library.get_sorted_gapic_configs():
build_file_folder = Path(f"{output_folder}/{gapic.proto_path}").resolve()
print(f"build_file_folder: {build_file_folder}")
gapic_inputs = parse_build_file(build_file_folder, gapic.proto_path)
Expand Down
45 changes: 45 additions & 0 deletions library_generation/model/gapic_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from library_generation.utils.proto_path_utils import find_version_in_proto_path

ALPHA_VERSION = "alpha"
BETA_VERSION = "beta"


class GapicConfig:
Expand All @@ -22,3 +26,44 @@ class GapicConfig:

def __init__(self, proto_path: str):
self.proto_path = proto_path

def __eq__(self, other) -> bool:
if not isinstance(other, GapicConfig):
return False
return self.proto_path == other.proto_path

def __lt__(self, other) -> bool:
self_version = find_version_in_proto_path(self.proto_path)
other_version = find_version_in_proto_path(other.proto_path)
# if only one config has a version in the proto_path, it is smaller
# than the other one.
if self_version and (not other_version):
return True
if (not self_version) and other_version:
return False

self_dirs = self.proto_path.split("/")
other_dirs = other.proto_path.split("/")
# if both of the configs have no version in the proto_path, the one
# with lower depth is smaller.
if (not self_version) and (not other_version):
return len(self_dirs) < len(other_dirs)

self_stable = GapicConfig.__is_stable(self_version)
other_stable = GapicConfig.__is_stable(other_version)
# if only config has a stable version in proto_path, it is smaller than
# the other one.
if self_stable and (not other_stable):
return True
if (not self_stable) and other_stable:
return False
# if two configs have different depth in proto_path, the one with
# lower depth is smaller.
if len(self_dirs) != len(other_dirs):
return len(self_dirs) < len(other_dirs)
# otherwise, the one with higher version is smaller.
return self_version > other_version
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit ignorant to string comparison in python. Is v2 greater than v1 because "2" has a higher ASCII value than "1"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is v2 greater than v1 because "2" has a higher ASCII value than "1"?

Yes.

I released that string comparison has a bug here: v10 is smaller than v2. I'll change the algorithm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


@staticmethod
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
def __is_stable(version: str) -> bool:
return not (ALPHA_VERSION in version or BETA_VERSION in version)
7 changes: 5 additions & 2 deletions library_generation/model/library_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# limitations under the License.
from hashlib import sha1

from typing import List, Optional
from typing import Optional
from library_generation.model.gapic_config import GapicConfig


Expand All @@ -29,7 +29,7 @@ def __init__(
api_description: str,
name_pretty: str,
product_documentation: str,
gapic_configs: List[GapicConfig],
gapic_configs: list[GapicConfig],
library_type: Optional[str] = None,
release_level: Optional[str] = None,
api_id: Optional[str] = None,
Expand Down Expand Up @@ -80,6 +80,9 @@ def get_library_name(self) -> str:
"""
return self.library_name if self.library_name else self.api_shortname

def get_sorted_gapic_configs(self) -> list[GapicConfig]:
return sorted(self.gapic_configs)

def __eq__(self, other):
return (
self.api_shortname == other.api_shortname
Expand Down
27 changes: 27 additions & 0 deletions library_generation/test/model/library_config_unit_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.
import unittest

from library_generation.model.gapic_config import GapicConfig
from library_generation.model.library_config import LibraryConfig


Expand All @@ -37,3 +38,29 @@ def test_get_library_returns_api_shortname(self):
gapic_configs=list(),
)
self.assertEqual("secret", library.get_library_name())

def test_get_sorted_gapic_configs_returns_correct_order(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we create a test for each of the scenario? [v1alpha1, v1], [v1, v2] etc. Because

  1. It would be much more readable. For example, just by reading the tests, I don't know why v1alpha1, v1, v2, admin_v2, non_versioned, v1beta1 is sorted to v2, v1, admin_v2, v1beta1, v1alpha1, non_versioned.
  2. We can also easily test more scenarios. e.g. scenarios of two non versioned libraries.

Copy link
Collaborator Author

@JoeWang1127 JoeWang1127 Jun 18, 2024

Choose a reason for hiding this comment

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

I added unit tests to verify comparison result of two gapic_configs.

I kept this test case though.

v1beta1 = GapicConfig(proto_path="google/spanner/v1beta1")
v1 = GapicConfig(proto_path="google/spanner/v1")
v1alpha1 = GapicConfig(proto_path="google/spanner/v1alpha")
v2 = GapicConfig(proto_path="google/spanner/v2")
admin_v2 = GapicConfig(proto_path="google/spanner/admin/v2")
non_versioned = GapicConfig(proto_path="google/spanner/type")
library = LibraryConfig(
api_shortname="secret",
name_pretty="",
product_documentation="",
api_description="",
gapic_configs=[v1alpha1, v1, v2, admin_v2, non_versioned, v1beta1],
)
self.assertEqual(
[
v2,
v1,
admin_v2,
v1beta1,
v1alpha1,
non_versioned,
],
library.get_sorted_gapic_configs(),
)
9 changes: 9 additions & 0 deletions library_generation/test/utils/proto_path_utils_unit_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from library_generation.utils.proto_path_utils import (
find_versioned_proto_path,
remove_version_from,
find_version_in_proto_path,
)

script_dir = os.path.dirname(os.path.realpath(__file__))
Expand Down Expand Up @@ -48,3 +49,11 @@ def test_remove_version_from_returns_non_versioned_path(self):
def test_remove_version_from_returns_self(self):
proto_path = "google/cloud/aiplatform"
self.assertEqual("google/cloud/aiplatform", remove_version_from(proto_path))

def test_find_version_proto_path_returns_version(self):
proto_path = "google/cloud/aiplatform/v1beta"
self.assertEqual("v1beta", find_version_in_proto_path(proto_path))

def test_find_version_proto_path_returns_none(self):
proto_path = "google/cloud/aiplatform"
self.assertIsNone(find_version_in_proto_path(proto_path))
9 changes: 9 additions & 0 deletions library_generation/utils/proto_path_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import re
from typing import Optional


def remove_version_from(proto_path: str) -> str:
Expand Down Expand Up @@ -45,3 +46,11 @@ def find_versioned_proto_path(proto_path: str) -> str:
idx = proto_path.find(version)
return proto_path[:idx] + version
return proto_path


def find_version_in_proto_path(proto_path: str) -> Optional[str]:
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
version_regex = re.compile(r"^v[1-9].*")
for directory in proto_path.split("/"):
if version_regex.search(directory):
return directory
return None
Loading