-
Notifications
You must be signed in to change notification settings - Fork 107
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
Move s3-credentials-endpoint to its own package #1685
Move s3-credentials-endpoint to its own package #1685
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.
This looks good, just a couple comments to talk about prior to approving:
- Node 12
- Ignoring the source map errors
@@ -220,7 +219,7 @@ async function ensureAuthorizedOrRedirect(req, res, next) { | |||
// Skip authentication for debugging purposes. | |||
// TODO Really should remove this | |||
if (process.env.FAKE_AUTH) { | |||
req.authorizedMetadata = { userName: randomId('username') }; | |||
req.authorizedMetadata = { userName: 'fake-auth-username' }; |
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.
Was there a reason this was randomized previously?
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.
No, it doesn't matter what this value is.
target: 'node' | ||
target: 'node', | ||
// https://github.com/webpack/webpack/issues/196#issuecomment-620227719 | ||
stats: { |
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.
Generically I don't know that I love ignoring this directly. That said, it's currently build spam for <checks log> got, express already. This probably warrants a quick chat, or a bit more digging on my part.
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.
From the maintainer of the keyv
package, which is generating this warning:
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.
Ok, sure, although there's this 'threadnaught' webpack/webpack#196 (that I'm not going to fully parse) where the webpack folks (at least from brief perusal) have some words (from years ago, to be fair) about that.
I'm fine not holding up the PR on this, but I'd like to make sure we at least have a quick arch discussion re: it to make sure the team is aware we're doing it.
@@ -8,6 +8,9 @@ locals { | |||
thin_egress_stack_name = "${var.prefix}-thin-egress-app" | |||
lambda_log_group_name = "/aws/lambda/${local.thin_egress_stack_name}-EgressLambda" | |||
tea_buckets = concat(var.protected_buckets, var.public_buckets) | |||
|
|||
repo_lambda_source_file = "${path.module}/../../packages/s3-credentials-endpoint/dist/lambda.zip" |
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.
This seems fine, although warrants a test before approving.
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.
On second thought, is there a good reason we shouldn't reverse the ternary here?
I would think the default should be if there's a built lambda.zip in the module path, we'd want that, given that's an explicit script we should never be running in dev except to test releases.
That should be fine, as ${path.module}/lambda.zip should exist in both distribution packages (and does by testing) due to L9 of build-tf-module.sh.
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.
Manually built/deployed a built .zip for the full cumulus package, and the stand-alone package looks actually correct now.
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.
On second thought, is there a good reason we shouldn't reverse the ternary here?
Fixed in cd4669b
Co-authored-by: Jonathan Kovarik <kovarik@nsidc.org>
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.
👍 looks good!
No functionality changes, just moving the JS code out of the TF module and into its own package.