Skip to content
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

chore: create logging module structure #3706

Merged
merged 16 commits into from
Mar 18, 2025
Merged

Conversation

JoeWang1127
Copy link
Collaborator

@JoeWang1127 JoeWang1127 commented Mar 17, 2025

In this PR:

  • Setup logging module structure.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Mar 17, 2025
@JoeWang1127 JoeWang1127 marked this pull request as ready for review March 17, 2025 14:24
@JoeWang1127 JoeWang1127 requested review from zhumin8 and blakeli0 March 17, 2025 14:24
pom.xml Outdated
@@ -24,6 +24,7 @@
<module>gapic-generator-java-bom</module>
<module>java-shared-dependencies</module>
<module>sdk-platform-java-config</module>
<module>java-sdk-logging</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this included in releases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the module is skipped during normal release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. Do you mind point me to where the normal release is setup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maven-deploy-plugin in these pom.xmls are skipped.

This is the same setup in the root pom.xml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it have to be added to the root pom?

Copy link
Collaborator Author

@JoeWang1127 JoeWang1127 Mar 17, 2025

Choose a reason for hiding this comment

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

No, it doesn't need to add to root pom.

Without this line, we need to specify the pom to build the module,

mvn clean install -f java-sdk-logging/pom.xml

However, we need to add a separate CI to ensure the tests are passing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer the tests for this module are a separate CI, since it has no dependencies on other modules at all. Even if the tests fail, it should not block the release of other modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'll create a separate ci.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@JoeWang1127 JoeWang1127 requested a review from zhumin8 March 17, 2025 14:50
<parent>
<groupId>com.google.api</groupId>
<artifactId>java-sdk-logging-parent</artifactId>
<version>0.1.0-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to keep the version name the same as the main project?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a standalone module so I don't think the version should be consistent with the main project and not all modules in this repo share the same version, though I don't feel strongly against it.

@blakeli0 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

A followup to this, if we keep this standalone with its own versions, what version should we start with? 0.1.0 seems to indicate unstable, do we want that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think 0.1.0 is fine now since we are still developing it.

We can change the version to 1.0.0 when we want to release the library.

pom.xml Outdated
@@ -24,6 +24,7 @@
<module>gapic-generator-java-bom</module>
<module>java-shared-dependencies</module>
<module>sdk-platform-java-config</module>
<module>java-sdk-logging</module>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it have to be added to the root pom?

@JoeWang1127 JoeWang1127 requested review from blakeli0 and zhumin8 March 17, 2025 16:50
Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

LGTM

<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-deploy-plugin</artifactId>
<configuration>
<skip>true</skip>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it is not part of the parent pom now, do we still have to skip this plugin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

- name: Install parent module
run: |
mvn install -B -ntp -pl gapic-generator-java-pom-parent
- name: Java Linter
Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition to linter, can we add clirr check and enforcer check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'll add them in a follow up PR.

Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

Discussed offline that CI check enhancements will be added in follow up PRs.

Copy link

Copy link

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

@JoeWang1127 JoeWang1127 enabled auto-merge (squash) March 18, 2025 16:42
@JoeWang1127 JoeWang1127 merged commit ce5998f into main Mar 18, 2025
54 of 56 checks passed
@JoeWang1127 JoeWang1127 deleted the chore/setup-logging-module branch March 18, 2025 16:43
JoeWang1127 added a commit that referenced this pull request Mar 18, 2025
A follow up of #3706:
- Change the group id to `com.google.cloud` as it is a customer-facing
library.
- Add clirr and enforcer check in CI.
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.

3 participants