Skip to content

chore: allow new-client.py to run directly without workspace#8653

Merged
meltsufin merged 5 commits into
mainfrom
new-client-changes
Oct 22, 2022
Merged

chore: allow new-client.py to run directly without workspace#8653
meltsufin merged 5 commits into
mainfrom
new-client-changes

Conversation

@meltsufin

Copy link
Copy Markdown
Contributor

No description provided.

@suztomo

suztomo commented Oct 21, 2022

Copy link
Copy Markdown
Member

Was there any problem with using workspace? I found using workspace (additional repository clone) is useful. Developing the script is simpler if we have 1 repository clone for new-client.py development and 1 repository clone for generated new client.

If you came up with a good way to develop new-client.py within 1 repository copy while testing library generation, I want to know the method (It's worth writing down in generation/new_client/README.md).

@meltsufin

Copy link
Copy Markdown
Contributor Author

Was there any problem with using workspace? I found using workspace (additional repository clone) is useful. Developing the script is simpler if we have 1 repository clone for new-client.py development and 1 repository clone for generated new client.

The problem with the workspace is that you can't make changes to the new-client.py script and test them without pushing upstream. It also makes it not hermetic. Additionally, there's overhead with checking out the whole repository first, when you already have it. Can you explain how development is easier with two different git repositories in one workspace?

If you came up with a good way to develop new-client.py within 1 repository copy while testing library generation, I want to know the method (It's worth writing down in generation/new_client/README.md).

This PR allows you to develop new-client.py within the same repository copy. That's exactly what I did while preparing this PR and generating #8646. I just excluded new-client.py from committing with the new module and created a separate PR.

@suztomo

suztomo commented Oct 21, 2022

Copy link
Copy Markdown
Member

you can't make changes to the new-client.py script and test them without pushing upstream.

In HOME/google-cloud-java you create a development branch for new-client.py (e.g., new-client-changes). You run new-client.py script and it creates HOME/google-cloud-java/generation/workspace/monorepo (e.g., new_module_java-discoveryengine). These two branches are separated; you don't have to push new-client-changes to main (upstream).

Can you explain how development is easier with two different git repositories in one workspace?

Having separate repository clones make the branches maintenance easier. 2 repository copy, 2 branches, and 2 pull requests.

I just excluded new-client.py from committing with the new module and created a separate PR.

Good. That works Can you add that instruction to README?

f" $ cd {monorepo_root}\n"
print(f"Please create a pull request:\n"
f" $ git checkout -b new_module_java-{output_name}\n"
f" $ git add .\n"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would include any change to new-client.py. Can you add an instruction to avoid that? With that instruction, next person who touches new-client.py will avoid committing changes to the new library creation request.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would someone need to modify new-client.py as part of a routine new library generation? However, even if they do, what's wrong with including it with the new library PR? It could help see the impact of the changes. We've been doing this kind of thing with every other script in generation directory, often including changes to scripts and their execution in the same PR.

@meltsufin

Copy link
Copy Markdown
Contributor Author

you can't make changes to the new-client.py script and test them without pushing upstream.

In HOME/google-cloud-java you create a development branch for new-client.py (e.g., new-client-changes). You run new-client.py script and it creates HOME/google-cloud-java/generation/workspace/monorepo (e.g., new_module_java-discoveryengine). These two branches are separated; you don't have to push new-client-changes to main (upstream).

The reason you have to push all the way to main is this line:

    subprocess.check_call(["git", "clone", monorepo_url, "monorepo"],
                          cwd="workspace")

It makes the starting point the main branch. Although new-client.py that is executed is in your local branch, it calls scripts that are coming from the remote main.

    subprocess.check_call(
        ["bash", "generation/update_owlbot_postprocessor_config.sh"],
        cwd=monorepo_root
    )

Where monorepo_root is the workspace directory that has code from main. So, you can't test changes to any of these files without pushing them upstream. It's not about new-client.py specifically, but other scripts it calls.

@meltsufin meltsufin marked this pull request as ready for review October 21, 2022 22:05
@meltsufin meltsufin added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 21, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 21, 2022
@meltsufin meltsufin added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 21, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 21, 2022
@meltsufin meltsufin merged commit a29b362 into main Oct 22, 2022
@meltsufin meltsufin deleted the new-client-changes branch October 22, 2022 00:26
@gcf-merge-on-green gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants