feat: add generation config comparator #2587

JoeWang1127 · 2024-03-16T00:44:33Z

In this PR:

Add a generation config comparator to compare the change type and which libraries are affected by the change.
Refactor LibraryConfig.
Add unit tests (no integration tests because the comparator has not been used).

The comparator makes assumptions that the following library level parameters will not change:

googleapis commit (no use case)

This is the first step to improve performance of hermetic code generation.

diegomarquezp

LGTM, thanks for this PR. Just a couple of minor comments.

library_generation/utils/generation_config_comparator.py

JoeWang1127 · 2024-03-19T01:57:33Z

library_generation/test/utils/generation_config_comparator_unit_tests.py

+        config_change = result[ChangeType.LIBRARIES_REMOVAL][0]
+        self.assertEqual("existing_library", config_change.library_name)
+
+    def test_compare_config_api_shortname_update(self):


Add this test case to verify the behavior of api_shortname change.

Why do we treat api_shortname change a library removal? If library_name stays the same and api_shortname changes, it should be a library level change?

If library_name stays the same and api_shortname changes, it should be a library level change?

In this test case, the library_name is changed because library_name is None and falls back to api_shortname.

Do you think we should add a test case to verify what happened if api_shorname is changed but library_name is set and remains the same? I expect this change will be a library level change.

Do you think we should add a test case to verify what happened if api_shorname is changed but library_name is set and remains the same? I expect this change will be a library level change.

Yes, please. Please rename this test case as well, as it currently only indicates api_shortname update. In addition, this makes me thinking that we should probably put the logic that figures out the library_name into GenerationConfig itself, because this logic is already somewhere in the scripts and we don't have to duplicate it again here.

we should probably put the logic that figures out the library_name into GenerationConfig itself

Is it better to put it into LibraryConfig?

Do you have a use case of changing api_shortname and does this case actual change it or just attempt to change it? This parameter is also in .OwlBot.yaml and I'm not sure what will happen if we only change this in configuration but not in .OwlBot.yaml.
cc: @diegomarquezp

Is it better to put it into LibraryConfig?

Yes, I meant LibraryConfig not GenerationConfig.

Do you have a use case of changing api_shortname and does this case actual change it or just attempt to change it? This parameter is also in .OwlBot.yaml and I'm not sure what will happen if we only change this in configuration but not in .OwlBot.yaml.

That's a good point, if we don't feel comfortable our scripts can handle api_shortname change, we should throw exceptions when we detect such changes.

I've moved get_library_name to LibraryConfig.

Added a test case to verify the script should raise ValueError when api_shorname is changed but library_name remains the same.

JoeWang1127 · 2024-03-19T01:57:50Z

library_generation/test/utils/generation_config_comparator_unit_tests.py

+        config_change = result[ChangeType.LIBRARIES_ADDITION][0]
+        self.assertEqual("new_api_shortname", config_change.library_name)
+
+    def test_compare_config_library_name_update(self):


Add this test case to verify the behavior of library_name change.

blakeli0 · 2024-03-19T20:38:56Z

library_generation/utils/generation_config_comparator.py

+    GOOGLEAPIS_COMMIT = 1
+    REPO_LEVEL_CHANGE = 2
+    LIBRARIES_ADDITION = 3
+    LIBRARIES_REMOVAL = 4


Is the scripts going to handle LIBRARIES_REMOVAL correctly? IIRC, the scripts is going to skip a library if it is not listed in the generation_config.yaml.

This PR only creates change types of two configurations, it doesn't specify how to handle each type.

For library removal, I expect generate_pr_body.py will create an release note entry like feat/chore: xxx is removed. However, the actual code in google-cloud-java needs to be manually removed.

Would it be better if we ignore this case and add the feat/chore: xxx is removed in the corresponding PR? Or we can think about how to remove libraries automatically, but it would probably be a larger effort.

Personally, I think it's better to output this change type here because this is the responsible of the comparator (to produce the change type, but not how to handle each type).

This type can serve as a trigger for automatic removal in further improvement and we don't need to refactor the comparator for this case. WDYT?

Sounds reasonable to me. But what is going to be the title for the PR that removes a library? Unless you want to make it a chore which would be a little confusing, it would still be duplicated in the release-note. I feel ignoring this case for now is better than documenting this behavior and tell release managers to not use feat in the removal PR. We don't have to remove the code, they can be commented out until we figure this out.

I commented out LIBRARIES_REMOVAL and GAPIC_REMOVAL.

I thought GAPIC_REMOVAL can be handled automatically?

@diegomarquezp after #2572 is merged, can we handle GAPIC removal automatically, i.e., deep-remove regex should work at library level?

Deep remove does not remove the entire library but the src folders inside that gapic, grpc and proto "sub libraries". It's meant to replace old code in the sublibraries. It wouldn't automatically delete any sublibrary if it stops being listed in the config yaml.

@blakeli0 does this answer your question?

blakeli0 · 2024-03-19T20:47:24Z

library_generation/utils/generation_config_comparator.py

+    if baseline_config.googleapis_commitish != latest_config.googleapis_commitish:
+        diff[ChangeType.GOOGLEAPIS_COMMIT] = []
+    if baseline_config.gapic_generator_version != latest_config.gapic_generator_version:
+        config_change = ConfigChange(


Can we iterate over all fields in GenerationConfig? Same thing for the LibraryConfig below. It could be a separate PR.

Can we iterate over all fields in GenerationConfig? Same thing for the LibraryConfig below.

Does the goal is to reduce the amount of code?

Yes, the logic is duplicated for every field.

To iterate field of a class, I think we need a enum type or a list of strings to specify the field?

Does something like this help?

I don't know python can do this, thanks for the help. I'll do some refactor then.

Done, use var to produce a mapping from parameter to its value.

…he same

JoeWang1127 · 2024-03-20T00:48:08Z

library_generation/test/utils/generation_config_comparator_unit_tests.py

+        config_change = result[ChangeType.LIBRARIES_ADDITION][0]
+        self.assertEqual("new_api_shortname", config_change.library_name)
+
+    def test_compare_config_api_shortname_update_with_library_name_raise_error(self):


Add this test case to verify the script should raise ValueError when api_shorname is changed but library_name remains the same.

sonarcloud · 2024-03-20T15:56:57Z

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

sonarcloud · 2024-03-20T16:00:11Z

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

In this PR: - Add `config_change.py` to get: - Changed libraries, which will be passed to `generate_repo.py` to generate libraries selectedly. - Qualified commits, which will be passed to `generate_pr_description.py` to generate PR description. - Refactor part of utility methods to `proto_path_utils.py`. - Refactor `generation_config_comparator.py` to move data class to `config_change.py`. - Refactor `utilities.py` and `utilities.sh` to `utils/utilities.py` and `utils/utilities.sh`. - Add unit tests. Follow up of #2587 For the goal of series of PRs, please refer to [improvement proposal](https://docs.google.com/document/d/1JiCcG3X7lnxaJErKe0ES_JkyU7ECb40nf2Xez3gWvuo/edit?tab=t.g3vua2kd06gx#bookmark=id.72s3ukwwzevo).

🤖 I have created a release *beep* *boop* --- <details><summary>2.39.0</summary> ## [2.39.0](v2.38.1...v2.39.0) (2024-04-18) ### Features * add `libraries_bom_version` to generation configuration ([#2639](#2639)) ([56c7ca5](56c7ca5)) * Add ChannelPoolSettings Getter for gRPC's ChannelProvider ([#2612](#2612)) ([d0c5191](d0c5191)) * add config change ([#2604](#2604)) ([8312706](8312706)) * add entry point ([#2616](#2616)) ([b19fa33](b19fa33)) * add generation config comparator ([#2587](#2587)) ([a94c2f0](a94c2f0)) * Add JavadocJar Task to build.gradle for self service libraries ([#2593](#2593)) ([993f5ac](993f5ac)) * Client/StubSettings' getEndpoint() returns the resolved endpoint ([#2440](#2440)) ([4942bc1](4942bc1)) * generate selected libraries ([#2598](#2598)) ([739ddbb](739ddbb)) * Validate the Universe Domain inside Java-Core ([#2592](#2592)) ([35d789f](35d789f)) ### Bug Fixes * add main to `generate_repo.py` ([#2607](#2607)) ([fedeb32](fedeb32)) * correct deep-remove and deep-preserve regexes ([#2572](#2572)) ([4c7fd88](4c7fd88)) * first attempt should use the min of RPC timeout and total timeout ([#2641](#2641)) ([0349232](0349232)) * remove duplicated calls to AutoValue builders ([#2636](#2636)) ([53a3727](53a3727)) * remove unnecessary slf4j and AbstractGoogleClientRequest native image configs ([0cb7d0e](0cb7d0e)) * remove unnecessary slf4j and AbstractGoogleClientRequest native image configs ([#2628](#2628)) ([0cb7d0e](0cb7d0e)) ### Dependencies * update arrow.version to v15.0.2 ([#2589](#2589)) ([777acf3](777acf3)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.28.0 ([#2649](#2649)) ([e4ed176](e4ed176)) * update dependency gitpython to v3.1.41 [security] ([#2625](#2625)) ([e41bd8f](e41bd8f)) * update dependency net.bytebuddy:byte-buddy to v1.14.13 ([#2646](#2646)) ([73ac5a4](73ac5a4)) * update dependency org.threeten:threeten-extra to v1.8.0 ([#2650](#2650)) ([226325a](226325a)) * update dependency org.threeten:threetenbp to v1.6.9 ([#2602](#2602)) ([371753e](371753e)) * update dependency org.threeten:threetenbp to v1.6.9 ([#2665](#2665)) ([8935bc8](8935bc8)) * update google api dependencies ([#2584](#2584)) ([cd20604](cd20604)) * update googleapis/java-cloud-bom digest to 7071341 ([#2608](#2608)) ([8d74140](8d74140)) * update netty dependencies to v4.1.109.final ([#2597](#2597)) ([8990693](8990693)) * update opentelemetry-java monorepo to v1.37.0 ([#2652](#2652)) ([f8fa2e9](f8fa2e9)) * update protobuf dependencies to v3.25.3 ([#2491](#2491)) ([b0e5041](b0e5041)) * update slf4j monorepo to v2.0.13 ([#2647](#2647)) ([f030e29](f030e29)) </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>

JoeWang1127 added 2 commits March 15, 2024 21:33

feat: add a generation config comparator

7f91d91

compare library configs

e5aae8e

product-auto-label bot added the size: l Pull request size is large. label Mar 16, 2024

JoeWang1127 added 11 commits March 16, 2024 00:45

Merge branch 'main' into feat/config-comparator

7110bb3

code refactor

7cb2e97

add proto_path removal or addition change

a8b1b26

change to HashLibrary

218e431

use object, rather than file path in comparator interface

15e0db7

add unit tests

b876354

add setUp method in ut

5348539

add unit tests against repo level changes

8e6b1c1

add unit tests against library parameters

4f0f2c1

add unit tests against versioned proto_path

e8b2d6b

restore gapic_config

e5f7a5e

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 18, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 18, 2024

add comments

2d6b13b

JoeWang1127 marked this pull request as ready for review March 18, 2024 13:40

JoeWang1127 requested a review from a team as a code owner March 18, 2024 13:40

JoeWang1127 requested review from blakeli0 and diegomarquezp March 18, 2024 13:40

diegomarquezp approved these changes Mar 18, 2024

View reviewed changes

library_generation/utils/generation_config_comparator.py Outdated Show resolved Hide resolved

library_generation/utils/generation_config_comparator.py Outdated Show resolved Hide resolved

code refactor

2dc41b3

blakeli0 reviewed Mar 18, 2024

View reviewed changes

library_generation/utils/generation_config_comparator.py Show resolved Hide resolved

blakeli0 reviewed Mar 18, 2024

View reviewed changes

library_generation/utils/generation_config_comparator.py Outdated Show resolved Hide resolved

blakeli0 reviewed Mar 18, 2024

View reviewed changes

library_generation/utils/generation_config_comparator.py Outdated Show resolved Hide resolved

change type

6fba2ad

JoeWang1127 requested a review from blakeli0 March 18, 2024 22:35

change enum types

cda24a1

JoeWang1127 commented Mar 19, 2024

View reviewed changes

blakeli0 reviewed Mar 19, 2024

View reviewed changes

blakeli0 mentioned this pull request Mar 19, 2024

feat: [shopping-merchant-quota] new module for shopping-merchant-quota googleapis/google-cloud-java#10575

Merged

JoeWang1127 added 5 commits March 19, 2024 23:41

move get_library_name method to LibraryConfig

af85a4f

fix integration tests

989da47

Merge branch 'main' into feat/config-comparator

d3df205

comment out library removal and gapic removal type

376015d

raise an error if api_shortname is changed but library_name remains t…

70e0fe8

…he same

JoeWang1127 commented Mar 20, 2024

View reviewed changes

JoeWang1127 added 3 commits March 20, 2024 15:05

use built-in method, vars, to list parameters of an object

42b5b03

Merge branch 'main' into feat/config-comparator

4890786

Merge branch 'main' into feat/config-comparator

33b8c47

blakeli0 approved these changes Mar 20, 2024

View reviewed changes

JoeWang1127 merged commit a94c2f0 into main Mar 21, 2024
31 checks passed

JoeWang1127 deleted the feat/config-comparator branch March 21, 2024 00:56

release-please bot mentioned this pull request Mar 20, 2024

chore(main): release 2.39.0 #2594

Merged

JoeWang1127 mentioned this pull request Mar 26, 2024

feat: add config change #2604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add generation config comparator #2587

feat: add generation config comparator #2587

JoeWang1127 commented Mar 16, 2024 •

edited

Loading

diegomarquezp left a comment

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024 •

edited

Loading

blakeli0 Mar 19, 2024

JoeWang1127 Mar 20, 2024

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 20, 2024

blakeli0 Mar 20, 2024

JoeWang1127 Mar 20, 2024

diegomarquezp Mar 20, 2024

JoeWang1127 Mar 20, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024

blakeli0 Mar 19, 2024

JoeWang1127 Mar 19, 2024

JoeWang1127 Mar 20, 2024

JoeWang1127 Mar 20, 2024

sonarcloud bot commented Mar 20, 2024

sonarcloud bot commented Mar 20, 2024

feat: add generation config comparator #2587

feat: add generation config comparator #2587

Conversation

JoeWang1127 commented Mar 16, 2024 • edited Loading

diegomarquezp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeWang1127 Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 20, 2024

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

sonarcloud bot commented Mar 20, 2024

Quality Gate passed for 'java_showcase_integration_tests'

JoeWang1127 commented Mar 16, 2024 •

edited

Loading

JoeWang1127 Mar 19, 2024 •

edited

Loading