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

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

Merged
merged 6 commits into from
Jun 19, 2024
Merged

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

merged 6 commits into from
Jun 19, 2024

Conversation

JoeWang1127
Copy link
Collaborator

@JoeWang1127 JoeWang1127 commented Jun 13, 2024 •
edited
Loading

In this PR:

  • populate .repo-metadata.json from highest stable version of proto paths.

The .repo-metadata.json is only generated once per library and the first item in proto paths is used, so the proto paths with the highest stable version should be the first item after sorting the proto paths in the library.

The method used to compare two proto paths, a and b:

  1. if both of a and b don't have a version, the one with lower depth is smaller.
  2. if either a or b has a version, the one with the version is smaller.
  3. if either a or b has a stable version, but not both, the one with the stable version is smaller, e.g., google/spanner/v2 is smaller than google/spanner/v3beta.
  4. if both a and b have a non-stable version, the one with higher version is smaller, e.g., google/spanner/v2beta1 is smaller than google/spanner/v2alpha2.
  5. if both a and b have a stable version, the one with lower depth is smaller, e.g., google/spanner/v1 is smaller than google/spanner/admin/v1, google/spanner/v1 is smaller than google/spanner/admin/v2
  6. if both a and b have a stable version and the depth is the same, the one with higher version is smaller, e.g., google/spanner/v2 is smaller than google/spanner/v1.

Test the change:

  1. build the image locally:
docker build -f .cloudbuild/library_generation/library_generation.Dockerfile -t local:latest .
  1. build the generator locally:
mvn -V -B -ntp clean install -DskipTests
  1. run the image:
docker run --rm -u "$(id -u):$(id -g)" -v "$(pwd):/workspace" -v "$HOME/.m2:/home/.m2" local:latest

Result:

  1. no change in common protos
  2. transport changed to both in java-iam/.repo-metadata.json. This is expected since the transport in google/iam/v2/BUILD.bazel is grpc+rest. Without this change, the transport is parsed from google/iam/v2/BUILD.bazel, which doesn't have a java_gapic_library target, thus the value is grpc by default.

@JoeWang1127 JoeWang1127 marked this pull request as ready for review June 17, 2024 16:19
@JoeWang1127 JoeWang1127 requested a review from a team as a code owner June 17, 2024 16:19
diegomarquezp
diegomarquezp reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

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

QQ, Don't we have .repo-metadata golden files for our units? Maybe we can change one of these to test the proto_path sorting criteria

library_generation/model/gapic_config.py Outdated
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.

blakeli0
blakeli0 reviewed Jun 18, 2024
View reviewed changes
library_generation/test/model/library_config_unit_tests.py
@@ -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 •
edited
Loading

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.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jun 18, 2024
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@JoeWang1127 JoeWang1127 requested a review from blakeli0 June 18, 2024 19:35
@JoeWang1127
Copy link
Collaborator Author

QQ, Don't we have .repo-metadata golden files for our units? Maybe we can change one of these to test the proto_path sorting criteria

The golden files are used in testing .repo-metadata.json generation, generate_prerequisite_files to be specific.

If we want to test generating this file with a library_config (from which contains multiple proto_path), we need test generate_composed_library, which is not easy to do since this function aggregates many "small" functions and is tested in integration test.

@JoeWang1127 JoeWang1127 merged commit f587541 into main Jun 19, 2024
34 checks passed
@JoeWang1127 JoeWang1127 deleted the feat/populate-metadate-from-highest-version branch June 19, 2024 12:19
@release-please release-please bot mentioned this pull request Jun 19, 2024
Merged
@blakeli0 blakeli0 mentioned this pull request Jun 20, 2024
Merged
JoeWang1127 added a commit that referenced this pull request Jun 25, 2024
@release-please @JoeWang1127
chore(main): release 2.42.0 (#2847)
4f5c4d9 
🤖 I have created a release *beep* *boop*
---


<details><summary>2.42.0</summary>

##
[2.42.0](v2.41.0...v2.42.0)
(2024-06-25)


### Features

* Allow Adding Client Level Attributes to MetricsTracerFactory
([#2614](#2614))
([f122c6f](f122c6f))
* gapic-generator-java to perform a no-op when no services are detected
([#2460](#2460))
([c0b5646](c0b5646))
* Make Layout Parser generally available in V1
([e508ae6](e508ae6))
* populate `.repo-metadata.json` from highest version
([#2890](#2890))
([f587541](f587541))
* push SNAPSHOT versions of the hermetic build docker image
([#2888](#2888))
([81df866](81df866))


### Bug Fixes

* **deps:** update the Java code generator (gapic-generator-java) to
1.2.3
([e508ae6](e508ae6))
* Expose Gax meter name
([#2865](#2865))
([6c5d6ce](6c5d6ce))
* Move the logic of getting systemProductName from static block to
static method
([#2874](#2874))
([536f1eb](536f1eb))
* Update default Otel Attribute from method_name to method
([#2833](#2833))
([af10a9e](af10a9e))


### Dependencies

* update dependency com.google.auto.value:auto-value to v1.11.0
([#2842](#2842))
([dd27fdf](dd27fdf))
* update dependency com.google.auto.value:auto-value-annotations to
v1.11.0
([#2843](#2843))
([bf8e67f](bf8e67f))
* update dependency com.google.cloud:grpc-gcp to v1.6.1
([#2943](#2943))
([9f16b40](9f16b40))
* update dependency org.checkerframework:checker-qual to v3.44.0
([#2848](#2848))
([7a99c50](7a99c50))
* update dependency org.easymock:easymock to v5.3.0
([#2871](#2871))
([c243f7d](c243f7d))
* update google api dependencies
([#2846](#2846))
([b5ef698](b5ef698))
* update googleapis/java-cloud-bom digest to 17cc5ec
([#2882](#2882))
([d6abd8e](d6abd8e))
* update netty dependencies to v4.1.111.final
([#2877](#2877))
([b5f10b9](b5f10b9))
* update opentelemetry-java monorepo to v1.39.0
([#2863](#2863))
([9d1f3a8](9d1f3a8))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Joe Wang <joewa@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakeli0 blakeli0 blakeli0 approved these changes

@diegomarquezp diegomarquezp Awaiting requested review from diegomarquezp

Assignees
No one assigned
Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JoeWang1127 @blakeli0 @diegomarquezp