Add server-aws-common to ZMS/ZTS assembly tarballs#3219
Add server-aws-common to ZMS/ZTS assembly tarballs#3219havetisyan merged 1 commit intoAthenZ:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical runtime issue for users deploying ZMS and ZTS in AWS environments. By incorporating a shared AWS dependency into the assembly build processes, it ensures that all required AWS-related classes are present in the final tarballs, preventing Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the athenz-server-aws-common dependency to the ZMS and ZTS assembly tarballs to resolve a ClassNotFoundException for users of AWS features. While this fixes the issue, it unconditionally adds large AWS SDK dependencies to the assemblies. My feedback suggests adopting the alternative approach mentioned in the PR description: using a Maven profile to conditionally include the AWS dependencies. This will keep the default distribution lean for users who do not require AWS integration, improving the user experience.
Note: Security Review has been skipped due to the limited scope of the PR.
| <dependency> | ||
| <groupId>${project.groupId}</groupId> | ||
| <artifactId>athenz-server-aws-common</artifactId> | ||
| <version>${project.parent.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
As you've noted in the pull request description, adding the AWS dependencies unconditionally increases the size of the assembly for all users, even those not using AWS. This can be a significant overhead for non-AWS deployments.
Your suggestion to use a Maven profile is the preferred approach. It allows users who need AWS support to build with a specific profile (e.g., -Paws), while keeping the default build lean.
Please refactor this to move the dependency into a profile. For example, in both assembly/zms/pom.xml and assembly/zts/pom.xml:
<profiles>
<profile>
<id>aws</id>
<dependencies>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>athenz-server-aws-common</artifactId>
<version>${project.parent.version}</version>
</dependency>
</dependencies>
</profile>
</profiles>
|
@gjoranv can you update both of yours to be signed. the failed DCO check has the details. thanks. |
gjoranv
force-pushed
the
include-server-aws-common-in-assembly
branch
from
2530b86 to
5ad4efa
Compare
- Add athenz-server-aws-common dependency to assembly/zms/pom.xml - Add athenz-server-aws-common dependency to assembly/zts/pom.xml Fixes AthenZ#3214 Signed-off-by: gjoranv <gjoranv@gmail.com>
gjoranv
force-pushed
the
include-server-aws-common-in-assembly
branch
from
5ad4efa to
a67c4b5
Compare
|
Done, thanks! |
2 participants
Summary
Adds
athenz-server-aws-commonas a dependency in bothassembly/zms/pom.xmlandassembly/zts/pom.xml. This ensures the AWS factory classes (AWSObjectStoreFactory,AWSCertRecordStoreFactory) and their transitive dependencies (AWS SDK v2) are included in the assembly tarballs.Without this, users deploying from the assembly tarballs as described in the ZMS setup docs get
ClassNotFoundExceptionat runtime when using the AWS factory classes.Fixes #3214
Trade-off
This adds the AWS SDK v2 JARs to both tarballs unconditionally, increasing their size. For non-AWS deployments these JARs are unnecessary. If that's a concern, an alternative approach would be a Maven profile (e.g.,
-Paws) that conditionally includes the dependency. Happy to implement that instead if preferred.