-
Notifications
You must be signed in to change notification settings - Fork 156
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
Patch lambda to allow imports #3420
Conversation
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
Actually thinking a bit more on the bad error message, I'm not aware of a way to both support the imported lambdas AND have the error about new lambdas with no code properties happen during validation though. |
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.
LGTM
Can you create a bridge issue to support ExactlyOneOf
in import?
|
||
// Check that we can change a property on the lambda | ||
secondStack.SetConfig("runtime", "nodejs16.x") | ||
secondStack.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.
There's no asserts here are we sure this is a strong test?
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.
The framework has the asserts by default: https://github.com/pulumi/providertest/blob/dc14d64784583753791733c6ac090d972d1ec746/pulumitest/up.go#L18
I've also verified that the test fails without the patch.
Tests are crucial here as the patches are a brittle mechanism an we are trying to move fast - so we need good reminders where it stops working. Would appreciate solutions without patches long-term e.g. lean into supported primitives if possible.. |
The issue is not the bridge support for We (and TF) previously supported creating a lambda without any of the code-related propeties (filename, s3_bucket, image_uri) but since the addition of the ExactlyOneOf this no longer works. Creating lambdas without code-related properties is necessary for imports. And for imports via the resource option we don't even have
I am not aware of any non-patch way of handling this, LMK if you disagree. |
How does pure TF handle this import. |
Badly: # __generated__ by Terraform
# Please review these resources and move them into your main configuration files.
# __generated__ by Terraform
resource "aws_lambda_function" "test_lambda" {
architectures = ["x86_64"]
code_signing_config_arn = null
description = "Lambda function that sets security headers on a Cloudfront origin response."
filename = null
function_name = "lambdaEdge-3d0ab09"
handler = "__index.handler"
image_uri = null
kms_key_arn = null
layers = []
memory_size = 128
package_type = "Zip"
publish = null
reserved_concurrent_executions = -1
role = "arn:aws:iam::616138583583:role/lambdaEdgeRole-8823a3f"
runtime = "nodejs12.x"
s3_bucket = null
s3_key = null
s3_object_version = null
skip_destroy = false
source_code_hash = "he1IIDJ8LmknDnaymtZVuMvwRjS8b2tCIxkpgwQlSe4="
tags = {}
tags_all = {}
timeout = 5
ephemeral_storage {
size = 512
}
tracing_config {
mode = "PassThrough"
}
}
However they don't have an import resource option, so this isn't that much of a regression for them - it only affects the |
Should address #2392
We regressed in our handling of imported lambdas in 5.28, when we picked up upstream https://github.com/hashicorp/terraform-provider-aws/releases/tag/v4.51.0 with https://github.com/hashicorp/terraform-provider-aws/pull/28963/files, which introduces the
ExactlyOneOf
constraints in the lambda resource.This PR reverts the upstream
ExactlyOneOf
constraints and replaces them with the previously appliedConflictsWith
. This in turn allows imports of lambdas viapulumi import
and theimport
resource option to work properly.I've added a GRPC test for a lambda imported via
pulumi import
as well as an integration test for a lambda imported via theimport
resource option and a test which checks that we still fail to create lambdas without any code-related properties (which should be the only case that now passes the validation step which previously didn't).Note that this slightly worsens the behaviour in the case when none of the code-related properties are specified. Previously that'd tirgger a failure during
Check
and print a sensible error message but now we will fail duringCreate
with the following much less legible error:I don't see a sensible way around it though and I think the change is still worthwhile.
Summary of effects: