-
Notifications
You must be signed in to change notification settings - Fork 53
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: selectively generate libraries #3290
Changes from 4 commits
3a577e3
69007f8
574b4a9
beb7aac
faf5fe8
7ec4bfa
b1b8784
82cef45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
# limitations under the License. | ||
import os | ||
import sys | ||
|
||
from typing import Optional | ||
import click as click | ||
from library_generation.generate_pr_description import generate_pr_descriptions | ||
from library_generation.generate_repo import generate_from_yaml | ||
|
@@ -51,6 +51,15 @@ def main(ctx): | |
metadata about library generation. | ||
""", | ||
) | ||
@click.option( | ||
"--includes", | ||
type=str, | ||
default=None, | ||
show_default=True, | ||
help=""" | ||
A list of library names that will be generated, separated by comma. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add more docs here? I think it is basically There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the comment. |
||
""", | ||
) | ||
@click.option( | ||
"--repository-path", | ||
type=str, | ||
|
@@ -77,33 +86,41 @@ def main(ctx): | |
def generate( | ||
baseline_generation_config_path: str, | ||
current_generation_config_path: str, | ||
includes: Optional[str], | ||
repository_path: str, | ||
api_definitions_path: str, | ||
): | ||
""" | ||
Compare baseline generation config and current generation config and | ||
generate changed libraries based on current generation config with commit | ||
history. | ||
If `includes` is specified, libraries whose name can be found in the current | ||
generation config will *also* be generated. | ||
|
||
If baseline generation config is not specified but current generation | ||
config is specified, generate all libraries based on current generation | ||
config without commit history. | ||
config is specified, generate all libraries if `includes` is not specified, | ||
based on current generation config without commit history. | ||
If `includes` is specified, only libraries whose name can be found in the | ||
current generation config will be generated. | ||
|
||
If current generation config is not specified but baseline generation | ||
config is specified, raise FileNotFoundError because current generation | ||
config should be the source of truth of library generation. | ||
|
||
If both baseline generation config and current generation config are not | ||
specified, generate all libraries based on the default generation config, | ||
which is generation_config.yaml in the current working directory. Raise | ||
FileNotFoundError if the default config does not exist. | ||
which is generation_config.yaml in the current working directory. | ||
If `includes` is specified, only libraries whose name can be found in the | ||
default generation config will be generated. | ||
Raise FileNotFoundError if the default config does not exist. | ||
|
||
The commit history, if generated, will be available in | ||
repository_path/pr_description.txt. | ||
""" | ||
__generate_repo_and_pr_description_impl( | ||
baseline_generation_config_path=baseline_generation_config_path, | ||
current_generation_config_path=current_generation_config_path, | ||
includes=includes, | ||
repository_path=repository_path, | ||
api_definitions_path=api_definitions_path, | ||
) | ||
|
@@ -112,6 +129,7 @@ def generate( | |
def __generate_repo_and_pr_description_impl( | ||
baseline_generation_config_path: str, | ||
current_generation_config_path: str, | ||
includes: Optional[str], | ||
repository_path: str, | ||
api_definitions_path: str, | ||
): | ||
|
@@ -146,14 +164,17 @@ def __generate_repo_and_pr_description_impl( | |
current_generation_config_path = os.path.abspath(current_generation_config_path) | ||
repository_path = os.path.abspath(repository_path) | ||
api_definitions_path = os.path.abspath(api_definitions_path) | ||
include_libraries = _parse_library_name_from(includes) | ||
|
||
if not baseline_generation_config_path: | ||
# Execute full generation based on current_generation_config if | ||
# Execute selective generation based on current_generation_config if | ||
# baseline_generation_config is not specified. | ||
# Do not generate pull request description. | ||
generate_from_yaml( | ||
config=from_yaml(current_generation_config_path), | ||
repository_path=repository_path, | ||
api_definitions_path=api_definitions_path, | ||
target_library_names=include_libraries, | ||
) | ||
return | ||
|
||
|
@@ -164,12 +185,17 @@ def __generate_repo_and_pr_description_impl( | |
baseline_config=from_yaml(baseline_generation_config_path), | ||
current_config=from_yaml(current_generation_config_path), | ||
) | ||
# pass None if we want to fully generate the repository. | ||
# Pass None if we want to fully generate the repository. | ||
target_library_names = ( | ||
config_change.get_changed_libraries() | ||
if not _needs_full_repo_generation(config_change=config_change) | ||
else None | ||
) | ||
# Generate the union of two library lists if neither of the library list | ||
# indicates a full generation. | ||
# The deduplication is done in `generate_from_yaml`. | ||
if (target_library_names is not None) and (include_libraries is not None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel this making the logic more complicated, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I also found it was a bit of confusing in this scenario. I changed this to |
||
target_library_names.extend(include_libraries) | ||
generate_from_yaml( | ||
config=config_change.current_config, | ||
repository_path=repository_path, | ||
|
@@ -191,6 +217,12 @@ def _needs_full_repo_generation(config_change: ConfigChange) -> bool: | |
return not current_config.is_monorepo() or current_config.contains_common_protos() | ||
|
||
|
||
def _parse_library_name_from(includes: str) -> Optional[list[str]]: | ||
if includes is None: | ||
return None | ||
return includes.split(",") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe trim the library name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
|
||
@main.command() | ||
@click.option( | ||
"--generation-config-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.
includes
is a little ambiguous to me that the user may not know what is the info we are including. For example, is thisapi_shortname
orlibrary_name
or something else? Justlibrary_names
ortarget_library_names
(we are already using it in scripts) sounds more reasonable to me.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 to
library_names
.