chore: migrate config change functions to common module
#3311
Conversation
product-auto-label
bot
added
size: l
| Compare baseline generation config with current generation config and | ||
| generate changed library names (a comma separated string) based on current | ||
| generation config. |
There was a problem hiding this comment.
| Compare baseline generation config with current generation config and | |
| generate changed library names (a comma separated string) based on current | |
| generation config. | |
| Compares baseline generation config with current generation config and | |
| generates changed library names (a comma separated string) based on current | |
| generation config. |
| current_generation_config_path = os.path.abspath(current_generation_config_path) | ||
| if not os.path.isfile(current_generation_config_path): | ||
| raise FileNotFoundError( | ||
| f"{current_generation_config_path} does not exist. " |
There was a problem hiding this comment.
nit: maybe abstract this duplicated exception message into a function/template? edit: not sure if the message is too small to not justify the effort.
| commit = commit_parents[0] | ||
| shutil.rmtree(tmp_dir, ignore_errors=True) | ||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| # we only need commit history, thus shadow clone is enough. |
There was a problem hiding this comment.
| # we only need commit history, thus shadow clone is enough. | |
| # we only need commit history, thus a shallow clone is enough. |
| @@ -92,6 +92,12 @@ pushd "${api_def_dir}" | |||
| git checkout "${googleapis_commitish}" | |||
| popd | |||
|
|
|||
| # generate changed libraries | |||
There was a problem hiding this comment.
I think generate changed libraries is a little confusing, I thought we are actually generating libraries when I first read it, but we are actually comparing two configs and figuring out the libraries to generate.
There was a problem hiding this comment.
Changed to get changed library list
There was a problem hiding this comment.
I would avoid using the term generation in this file. generate_repo and generate_pr_description make sense because we are actually generating something as files. generate_changed_library though is not really generating something as files, it is an intermediate step to generate_repo and generate_pr_description. The file name could be just get_changed_libraries.
| from common.utils.generation_config_comparator import compare_config | ||
|
|
||
|
|
||
| @click.group(invoke_without_command=False) |
There was a problem hiding this comment.
Since this file is not supposed to be called directly, does it still have to be a cli? Maybe cli is still easier, but if not, can we call it as a Python function directly?
There was a problem hiding this comment.
We need to use this function in shell script, so I add a CLI to simplify the call.
The result is passed to entry_point.py.
There was a problem hiding this comment.
This function is used in shell script so I add a CLI to simplify the call.
| def _parse_library_name_from( | ||
| includes: str, generation_config: GenerationConfig | ||
| ) -> Optional[list[str]]: | ||
| if includes is None or _needs_full_repo_generation(generation_config): |
There was a problem hiding this comment.
Is this _needs_full_repo_generation(generation_config) a logic change?
There was a problem hiding this comment.
No, this is an existing logic.
There was a problem hiding this comment.
It was not here before, is it moved from somewhere?
There was a problem hiding this comment.
This function determines whether we want a full generation, i.e., we want a full generation if it's a split repo or sdk-platform-java.
The logic is there before but I moved it here to help create the library name list. If the user doesn't specify --library-names or we do want a full generation, returns None, which means full generation.
| ) | ||
| @click.option( | ||
| "--current-generation-config-path", | ||
| "--generation-config-path", |
There was a problem hiding this comment.
I think this means our library generation would not need internet access anymore?
|
|
In this PR:
commonmodule.