-
Notifications
You must be signed in to change notification settings - Fork 51
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: add generation config comparator #2587
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this PR. Just a couple of minor comments.
config_change = result[ChangeType.LIBRARIES_REMOVAL][0] | ||
self.assertEqual("existing_library", config_change.library_name) | ||
|
||
def test_compare_config_api_shortname_update(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this test case to verify the behavior of api_shortname
change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
config_change = result[ChangeType.LIBRARIES_ADDITION][0] | ||
self.assertEqual("new_api_shortname", config_change.library_name) | ||
|
||
def test_compare_config_library_name_update(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this test case to verify the behavior of library_name
change.
GOOGLEAPIS_COMMIT = 1 | ||
REPO_LEVEL_CHANGE = 2 | ||
LIBRARIES_ADDITION = 3 | ||
LIBRARIES_REMOVAL = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented out LIBRARIES_REMOVAL
and GAPIC_REMOVAL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought GAPIC_REMOVAL
can be handled automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diegomarquezp after #2572 is merged, can we handle GAPIC removal automatically, i.e., deep-remove regex should work at library level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blakeli0 does this answer your question?
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we iterate over all fields in GenerationConfig
? Same thing for the LibraryConfig
below. It could be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we iterate over all fields in GenerationConfig? Same thing for the LibraryConfig below.
Does the goal is to reduce the amount of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the logic is duplicated for every field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To iterate field of a class, I think we need a enum type or a list of strings to specify the field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does something like this help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know python can do this, thanks for the help. I'll do some refactor then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, use var
to produce a mapping from parameter to its value.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this test case to verify the script should raise ValueError
when api_shorname
is changed but library_name
remains the same.
|
|
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).
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>
🤖 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>
In this PR:
LibraryConfig
.The comparator makes assumptions that the following library level parameters will not change:
This is the first step to improve performance of hermetic code generation.