Add initial architecture rules #5741
Conversation
zoewangg
force-pushed
the
zoewang/addArchitectureTestsModule
branch
11 times, most recently
from
aee08c7 to
c17e892
Compare
| import software.amazon.awssdk.annotations.SdkPublicApi; | ||
|
|
||
| @AnalyzeClasses(packages = "software.amazon.awssdk..") | ||
| @ArchIgnore(reason = "CI keeps crashing when running the tests. Ignoring them for now") |
There was a problem hiding this comment.
Had to ignore tests in this class because CI keeps crashing for some reason. Will take a look and follow up in a separate PR
zoewangg
changed the title
| * @param maybeCloseable the given closeable | ||
| * @param log logger used to log any failure should the close fail | ||
| */ | ||
| public static void closeIfCloseableV2(Object maybeCloseable, Logger log) { |
There was a problem hiding this comment.
We can't use the same signature because it'll break people who invoke closeIfClosable(closable, null)
| * @param is the given closeable | ||
| * @param log logger used to log any failure should the close fail | ||
| */ | ||
| public static void closeQuietlyV2(AutoCloseable is, Logger log) { |
There was a problem hiding this comment.
We can't use the same signature because it'll break people who invoke closeQuietly(closable, null)
| <dependency> | ||
| <artifactId>dynamodb-enhanced</artifactId> | ||
| <groupId>software.amazon.awssdk</groupId> | ||
| <version>${awsjavasdk.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
How do we choose to add a dependency here, vs just including everything? Does it save time to just include things with customizations?
There was a problem hiding this comment.
Yes, saving build time and avoiding oom error (I think the library has memory leak somewhere) is the main reason. I'm planning to add this to the new module checklist. Ideally, we should run this test on all handwritten modules as well as service modules with heavy customization.
test/architecture-tests/src/test/java/software/amazon/awssdk/archtests/ArchUtils.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static String findRootPackage(String pkg) { | ||
| if (pkg.startsWith("software.amazon.awssdk.services.")) { |
There was a problem hiding this comment.
Do we need to treat "services" differently? It feels like from where we're using this method in this PR, it wouldn't need to be treated specially.
There was a problem hiding this comment.
Yes, because the root component for service package is the the last component after software.amazon.awssdk.services. whereas for core modules, it's the one after software.amazon.awssdk.awssdk. For example. softare.amazon.awssdk.services.s3 and software.amazon.awssdk.services.rds are not under the same root.
test/architecture-tests/src/test/java/software/amazon/awssdk/archtests/ArchUtils.java
Outdated
Show resolved
Hide resolved
zoewangg
force-pushed
the
zoewang/addArchitectureTestsModule
branch
from
b1a07bb to
630444c
Compare
zoewangg
force-pushed
the
zoewang/addArchitectureTestsModule
branch
from
12df7e8 to
a03a412
Compare
|
Motivation and Context
Codify architecture rules, coding conventions and naming conventions and ensure they are followed.
Modifications
Create a new test module
architecture-teststo add the following rules:Package conventions
@SdkInternalApi) MUST reside in ”..internal..” subpackage@SdkPublicApi) and Protected APIs (@SdkProtectedApi) MUST NOT reside in “..internal..” subpackage@SdkInternalApi) MUST NOT be used across modulesCoding conventions
Naming conventions
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License