Skip to content

chore: always fully generate the repo if it is a non-monorepo or contains common protos#3162

Merged
JoeWang1127 merged 8 commits intomainfrom
chore/always-gen-if-contains-common
Sep 4, 2024
Merged

chore: always fully generate the repo if it is a non-monorepo or contains common protos#3162
JoeWang1127 merged 8 commits intomainfrom
chore/always-gen-if-contains-common

Conversation

@JoeWang1127
Copy link
Contributor

@JoeWang1127 JoeWang1127 commented Sep 3, 2024

In this PR:

This PR also brings changes in common protos and iam due to protoc updates (25.3 -> 25.4).

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 3, 2024
@JoeWang1127 JoeWang1127 marked this pull request as ready for review September 3, 2024 23:10
@JoeWang1127 JoeWang1127 changed the title chore: always fully generate the repo if it contains common protos chore: always fully generate the repo if it's a non-monorepo or it contains common protos Sep 3, 2024
@JoeWang1127 JoeWang1127 changed the title chore: always fully generate the repo if it's a non-monorepo or it contains common protos chore: always fully generate the repo if it is a non-monorepo or contains common protos Sep 3, 2024
@blakeli0
Copy link
Contributor

blakeli0 commented Sep 3, 2024

This PR also brings changes in common protos and iam due to protoc updates (25.3 -> 25.4).

Can we leave this part to the nightly generation?

@JoeWang1127
Copy link
Contributor Author

JoeWang1127 commented Sep 3, 2024

Can we leave this part to the nightly generation?

When I changed the workflow, the workflow brings this change. It's not a manual commit.

I think it's good because it verified the expected behavior.

In addition, the nightly generation should only bring changes with respect to googleapis commit, not protoc change in Dockerfile.

# does special handling when a method is annotated with @main.command()
generate_impl(
baseline_generation_config_path=baseline_config_path,
current_generation_config_path=current_config_path,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the same config path? I think it should not matter if the two configs are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is verifying the list of changed library can be populated in a monorepo.

If we use two identical configs, the list should be empty, i.e., no changed library. I think this is fine as long as the list is not None.

I'll make the change.

Copy link
Contributor

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit.

@@ -163,6 +163,15 @@ def __generate_repo_and_pr_description_impl(
)


def __needs_full_repo_generation(config_change: ConfigChange) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: private functions are recommended to have single underscores

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the function name in this file.

We can gradually change functions in other files in future PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @diegomarquezp, TIL.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 4, 2024

Quality Gate Failed Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
10.1% Duplication on New Code (required ≤ 3%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 4, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -66,13 +79,13 @@ def _run_command_and_get_sdout(self, command, **kwargs):
def test_get_grpc_version_with_no_env_var_fails(self):
# the absence of DOCKER_GRPC_VERSION will make this function to fail
result = self._run_command("get_grpc_version")
self.assertEquals(1, result.returncode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assertEquals is deprecated so I changed it to assertEqual.

@JoeWang1127 JoeWang1127 merged commit 7f0ccc6 into main Sep 4, 2024
@JoeWang1127 JoeWang1127 deleted the chore/always-gen-if-contains-common branch September 4, 2024 19:01
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
…ains common protos (#3162)

In this PR:
- Always perform a full generation in a non-monorepo or the repo
contains common protos.
- Secure generation workflow (use environment variable to avoid script
injection), inspired by
googleapis/java-bigtable#2317

This PR also brings changes in common protos and iam due to protoc
updates (25.3 -> 25.4).

---------

Co-authored-by: cloud-java-bot <cloud-java-bot@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants