-
Notifications
You must be signed in to change notification settings - Fork 54
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 entry point #2616
feat: add entry point #2616
Conversation
""", | ||
) | ||
@click.option( | ||
"--latest-generation-config", |
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 all it something more generic?Because theoretically we can compare any two configs, not always compare with the latest.
In addition, what if the latest-generation-config
has a googleapis commitish that is prior to it in basedline-generation-config
? I don't think we have to support this case, but we should at least handle this case so our script does not blow up, maybe by adding validations that the googleapis commitish in one of them should be higher than the other.
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 all it something more generic?Because theoretically we can compare any two configs, not always compare with the latest.
I think these two names are intuitive, do you have other candidates?
what if the latest-generation-config has a googleapis commitish that is prior to it in basedline-generation-config?
The problem is we don't the relationship between two commitish without traversing from one to another. The worst scenario is the description has too many characters (due to lots of commits) and gh
command returns error.
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 decided to use current_generation_config
and baseline_generation_config
as parameter names.
The googleapis commitish in current_generation_config
should be newer than baseline_generation_config
and we'll verify this by commit time.
@main.command() | ||
@click.option( | ||
"--baseline-generation-config", | ||
required=True, |
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 make the interface as simple as possible? For example, both of the configs can be optional. We can use the existence of baseline config to decide if we want to to generate PR descriptions, and assume the current config to be exist in the current folder?
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 both are optional, then we have:
- both are not specified, we assume there's a
generation_config.yaml
in the current dir and do not generate pr description. - one is specified, we use it to generate repo and do not generate pr description.
- both are specified, we use
current-generation-config
to generate repo and generate pr description.
if not os.path.isfile(default_generation_config): | ||
raise FileNotFoundError( | ||
f"{default_generation_config} does not exist when " | ||
f"both baseline_generation_config and current_generation_config" |
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 additional info may be more complete but may also confuse people, took me sometime to understand as well. Can we simply saying
f"{default_generation_config} does not exist.
or
f"{default_generation_config} does not exist. A valid generation config has to be passed in as "current_generation_config" or exist in current working directory.
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.
default=None, | ||
type=str, | ||
help=""" | ||
Path to generation_config.yaml that contains the metadata about |
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.
Absolute path?
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.
Relative path is fine because all path will be converted to absolute path later.
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.
So either relative path or absolute path should work? If that's the case, let's document it here as well.
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.
help=""" | ||
Path to generation_config.yaml that contains the metadata about | ||
library generation. | ||
The googleapis commit in the configuration is the oldest commit, |
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 include some high level description for baseline-generation-config
? For example, something like "We compare baseline-generation-config with the current generation config, and generate commit messages based on the diff of the googleapis-commitish between the two generation configs."
Same comment for current-generation-config
below.
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 added high level description of generate
method in doc string.
User can read the docs through
python library_generation/cli/entry_point.py generate --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.
Thanks, looks good! I think we still need more info to distinguish between baseline-generation-config
from current-generation-config
. For example, both descriptions include Absolute or relative path to generation_config.yaml that contains the metadata about library generation.
, in fact, only the info in current-generation-config
is used for generation, baseline-generation-config
is only used for generating commit history. Also the additional info regarding inclusive/exclusive of googleapis commitish might be a little too detailed.
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.
Changed the comment.
commit = repo.commit(current_commit) | ||
current_commit_time = __get_commit_timestamp(commit) | ||
baseline_commit_time = __get_commit_timestamp(repo.commit(baseline_commit)) | ||
if current_commit_time < baseline_commit_time: |
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.
Like this validation! I think we should add =
though.
if current_commit_time < baseline_commit_time: | |
if current_commit_time <= baseline_commit_time: |
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.
Added unit tests to verify the behavior.
repository_path=repository_path, | ||
target_library_names=config_change.get_changed_libraries(), | ||
) | ||
generate_pr_descriptions( |
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 probably want to rename generate_pr_description
to something like generate_commit_history
, because we just happen to use PR description to capture the commit history, and we could use other ways. It could be a separate PR.
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
# generate the repository | ||
python -m library_generation/generate_repo.py generate \ | ||
--generation-config-yaml=/path/to/config-file \ | ||
python -m library_generation/entry_point.py generate \ |
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.
FYI, we have to remove -m
here, this is what tripped me when I run locally, I updated it in my PR
🤖 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:
entry_point.py
to combinegenerate_repo.py
andgenerate_pr_description.py
.Follow up of #2604.
For the goal of series of PRs, please refer to improvement proposal.