Skip to content

Conversation

0marperez
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:
Adds actions that can be used downstream to deal with artifact size metrics

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@0marperez 0marperez marked this pull request as ready for review October 3, 2025 15:39
@0marperez 0marperez requested a review from a team as a code owner October 3, 2025 15:39
Comment on lines 5 to 8
upload:
description: Whether the metrics should be uploaded to S3/Cloudwatch
release_metrics:
description: Whether the metrics are coming from a release build
Copy link
Member

Choose a reason for hiding this comment

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

correctness: configure type: boolean

Comment on lines +40 to +43
# Check for large diff
if [ "$percent" -gt 5 ]; then
large_diff=true
fi
Copy link
Member

Choose a reason for hiding this comment

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

Are we no longer using the significantChangeThresholdPercentage configured in the plugin?

/**
* The threshold for an acceptable artifact size increase (percentage)
*/
var significantChangeThresholdPercentage: Double = 5.0

Let's delete it there if it's no longer being used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, forgot to delete all that. We never really changed the value from 5% so it doesn't seem necessary.

echo "Artifact, Size (Bytes)" > "$output_file"

# Find all JARs (exclude sources and javadoc)
# TODO: Calculate KN artifacts sizes
Copy link
Member

Choose a reason for hiding this comment

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

do we have an internal task for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have an older task for it: SDK-KT-771

if [ "$GITHUB_REPOSITORY" = "aws-sdk-kotlin" ]; then
# FIXME: Enable K/N builds
./gradlew build -Paws.kotlin.native=false build --parallel --max-workers 16
./gradlew -Paws.kotlin.native=false publish --parallel --max-workers 16
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be publishAllPublicationsToTestLocalRepository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be the same, our publishing configuration applies to AbstractPublishToMaven tasks. I can do some more testing and update this if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to publishAllPublicationsToTestLocalRepository in the codebuild job btw

Comment on lines +52 to +57
// Minimize the comment
await github.graphql(`
mutation {
minimizeComment(input: {subjectId: "${commentId}", classifier: RESOLVED}) { }
}
`)
Copy link
Member

Choose a reason for hiding this comment

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

Are we always minimizing the comment now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we rarely have to actually look at the metrics. If we do, it's really easy to just un-minimize a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Aren't the artifact sizes logged in the CI output? Maybe we don't need the comment at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants