Contributes Maven ArtifactSizeEnforcerRule to DDF-Support #54
Conversation
coyotesqrl
requested review from
shaundmorris,
avalexxx,
mcalcote and
peterhuffer
| <rules> | ||
| <rule> | ||
| <element>BUNDLE</element> | ||
| <limits> |
There was a problem hiding this comment.
❓ Should we not use lenient limits?
| long maxArtifactSizeBytes = maxArtifactSizeToBytes(helper); | ||
| long artifactSize = getArtifact(artifactPath).length(); | ||
|
|
||
| if (artifactSize >= maxArtifactSizeBytes) { |
There was a problem hiding this comment.
❓ Should we be lenient a bit? Since different compilers on different OS (or the same OS) might be generating different byte code. Hardcoding a hard limit might suffer the same fate as did jacoco.
There was a problem hiding this comment.
My feeling is that we should be less worried about this particular failure hitting us only because of OS differences than Jacoco. Even if we're bumping up near the size boundary, the developer should be paying attention and should fix the size of the output artifact or manually bump the allowed max size. In reality, we shouldn't be close to the limits unless we're embedding jars willy-nilly.
@tbatie what's your opinion on this?
| case JAR: | ||
| return JAR; | ||
| case BUNDLE: | ||
| return JAR; |
| } | ||
|
|
||
| if (convertArtifactSize.endsWith(BYTES)) { | ||
| return Long.parseLong(convertArtifactSize.split(BYTES)[0]); |
There was a problem hiding this comment.
❓ Should we catch the NumberFormatException that can occur?
| enforcer.setMaxArtifactSize("INVALID_UNIT"); | ||
| enforcer.execute(defaultMockhelper); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add tests for invalid parsing for _B, _KB, and _MB values
|
Updates for code review comments pushed in separate commit. |
| .debug( | ||
| String.format(DEFAULT_ARTIFACT_INFO_MSG, artifactId, version, packaging, buildDir)); | ||
|
|
||
| convertedArtifactLocation = |
There was a problem hiding this comment.
❓ Is there a reason you didn't use String.format() here?
There was a problem hiding this comment.
No reason at all; I'll change it.
| <version>2.3.15-SNAPSHOT</version> | ||
| </parent> | ||
|
|
||
| <artifactId>artifact-size-enforcer</artifactId> |
There was a problem hiding this comment.
✏️ <name> ? <description> may be handy too if we are looking for more third parties to use this.
| import org.codehaus.plexus.util.StringUtils; | ||
|
|
||
| /** | ||
| * When the ArtifactSizeEnforceRule is active, it will look up the packaging type and enforce a |
| long maxArtifactSizeBytes = maxArtifactSizeToBytes(helper); | ||
| long artifactSize = getArtifact(artifactPath).length(); | ||
|
|
||
| if (artifactSize >= maxArtifactSizeBytes) { |
There was a problem hiding this comment.
❓ Did we want this comparison to be greater than or equal to, or just greater than? The exception message indicates the size should not be exceeded.
| */ | ||
| public class ArtifactSizeEnforcerRule implements EnforcerRule { | ||
|
|
||
| static final String PROJECT_PACKAGING_PROP = "${project.packaging}"; |
There was a problem hiding this comment.
❓ Worth adding @VisibleForTesting on all these?
| packaging); | ||
| helper | ||
| .getLog() | ||
| .debug( |
There was a problem hiding this comment.
Nit, any reason we don't log the generated path at the info level as you are when a path is provided? If this plugin fails, this could be really useful information to the user.
| "Using the following parameters to find artifact: %n\tArtifactId: %s%n\tVersion: %s%n\tPackaging: %s%n\tArtifact Directory: %s"; | ||
|
|
||
| private static final String UNKNOWN_ARTIFACT_SIZE_UNIT_MSG = | ||
| "Unknown artifact size unit. The artifactSize property must end with either: %n\t%s: Bytes%n\t%s: KiloBytes%n\t%s: MegaBytes"; |
There was a problem hiding this comment.
✏️ I think we should tell the user they must end in the constants we're actually checking for here https://github.com/codice/ddf-support/pull/54/files#diff-d4e3d19b536f36abc3f7081665cc04d6R236
There was a problem hiding this comment.
We do. It was just being oddly constructed before; I've simplified it.
- also restructures support-maven module to more easily enable future additions
|
build now |
|
Internal build has been scheduled, your results will be available at build completion. |
|
Refer to this link for build results (access rights to CI server needed): |
Also restructures support-maven module to more easily enable future additions and resolves static analysis findings.
Git does not recognize the move of
MavenVersionValidationPluginbecause it detects too many internal changes; these were just fixes to static findings.