googleapis / sdk-platform-java

Tooling and shared libraries for Cloud SDK for Java
https://cloud.google.com/java/docs/bom
Apache License 2.0
68 stars 53 forks source link

chore: remove protoc grpc from generation config #3390

Closed JoeWang1127 closed 1 day ago

JoeWang1127 commented 6 days ago

In this PR:

JoeWang1127 commented 4 days ago

I know we said that it's OK to not support running scripts directly. What is the effort to support it? Do we need to write additional code or just need to set DOCKER_PROTOC_VERSION and DOCKER_GRPC_VERSION?

I think we need to download the the protoc/grpc and put them into a well-known location, just like the generator and formatter jar.

diegomarquezp commented 4 days ago
  • Remove instructions on running python script in local environment because the current setup will fail if DOCKER_PROTOC_VERSION or DOCKER_GRPC_VERSION is not set.

We don't write any additional code to support running the scripts directly.

diegomarquezp commented 4 days ago
  • Remove instructions on running python script in local environment because the current setup will fail if DOCKER_PROTOC_VERSION or DOCKER_GRPC_VERSION is not set.

@JoeWang1127 why not just explain the developer that these variables must be set?

blakeli0 commented 4 days ago

I think we need to download the the protoc/grpc and put them into a well-known location, just like the generator and formatter jar

Cool, if that's the case, maybe we can continue to support it by adding a few documentation to DEVELOPMENT.md? Feel free to not do it if you think there is not much use case for it anyway and it's not worth the effort.

JoeWang1127 commented 4 days ago

@JoeWang1127 why not just explain the developer that these variables must be set?

I don't think it's intuitive to set these environment variables only for running the script directly.

The variable name is clearly indicating running in docker container, why setting these env if we're using python script?

diegomarquezp commented 4 days ago

@JoeWang1127 why not just explain the developer that these variables must be set?

I don't think it's intuitive to set these environment variables only for running the script directly.

The variable name is clearly indicating running in docker container, why setting these env if we're using python script?

I agree it's not intuitive. However, these variables are automatically set when building the image, so we must set them always, although manually for the case of local development.

I remember we used the DOCKER_ prefix was used to distinguish the variables from what's in the config file, but we are not using these config entries in the yaml since this PR. Maybe we can drop the prefix and rename them to PROTOC_LOCATION? edit: this can be a follow up

JoeWang1127 commented 4 days ago

Cool, if that's the case, maybe we can continue to support it by adding a few documentation to DEVELOPMENT.md? Feel free to not do it if you think there is not much use case for it anyway and it's not worth the effort.

I think we can create another pr to support the directly run. I feel it's too much for this PR.

I remember we used the DOCKER_ prefix was used to distinguish the variables from what's in the config file, but we are not using these config entries in the yaml since this PR. Maybe we can drop the prefix and rename them to PROTOC_LOCATION? edit: this can be a follow up

SGTM.

JoeWang1127 commented 1 day ago

I decided to keep the support of running python script directly in development guide.

sonarcloud[bot] commented 1 day ago

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

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 SonarQube Cloud

sonarcloud[bot] commented 1 day ago

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 SonarQube Cloud