-
Notifications
You must be signed in to change notification settings - Fork 14
chore: Refactor to build into single artifact #74
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment otherwise looks pretty good
# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
# These make targets that are shared | ||
# between all the projects in this repo. | ||
# They will only function if executed inside a project directory. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have this include the root one and then modify it accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but just duplicating it here was easier. I would prefer to backfill the one in MPL to match the one here.
Realistically, it's only for convenience/to avoid duplication that the SharedMakefile belongs to MPL. The way I think of this is a bit similar to gradle, where each smithy-dafny project would come with some canonical SharedMakefile that gives you most of the goodness you are already looking for. And this "SharedMakefile" exists in multiple places just because each project needs access to it, and shoudn't have to dig into it's dependencies to do so.
run: | | ||
# This works because `node` is installed by default on GHA runners | ||
CORES=$(node -e 'console.log(os.cpus().length)') | ||
make verify CORES=$CORES | ||
make verify_namespace CORES=$CORES SMITHY_NAMESPACE=${{ matrix.namespace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new target because you need to break it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, otherwise it takes >30min for CI to turn green. I figure once we launch and aren't doing as much development on it, we could switch back to the single target to be extra sure we aren't missing any files.
Refactor contains the following major changes:
DynamoDbEncryption
and updated Makefile to handle building multiple namespacesdynamoDbEncryption.transforms
dynamoDbEncryption.itemEncryptor
.Manual smithy change added:
Manual smithy changes preserved:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.