Skip to content
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

resource/aws_lambda_function: support Java 11, Nodejs12x, Python3.8 runtimes #10938

Merged
merged 2 commits into from
Nov 20, 2019

Conversation

ethanrubio
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #10919.
Reference: https://github.com/aws/aws-sdk-go/releases/tag/v1.25.38.

Release note for CHANGELOG:

* resource/aws_lambda_function: support Java 11, Nodejs12x, Python3.8 runtimes

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSLambdaFunction_runtimeValidation_java11'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSLambdaFunction_runtimeValidation_java11 -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSLambdaFunction_runtimeValidation_java11
=== PAUSE TestAccAWSLambdaFunction_runtimeValidation_java11
=== CONT  TestAccAWSLambdaFunction_runtimeValidation_java11
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_java11 (44.21s)
PASS

$ make testacc TESTARGS='-run=TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x'
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x
=== PAUSE TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x
=== CONT  TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x (42.49s)
PASS

$ make testacc TESTARGS='-run=TestAccAWSLambdaFunction_runtimeValidation_python38'
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSLambdaFunction_runtimeValidation_python38 -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSLambdaFunction_runtimeValidation_python38
=== PAUSE TestAccAWSLambdaFunction_runtimeValidation_python38
=== CONT  TestAccAWSLambdaFunction_runtimeValidation_python38
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python38 (44.24s)
PASS
...

@ethanrubio ethanrubio requested a review from a team November 19, 2019 20:37
@ghost ghost added needs-triage Waiting for first response or review from a maintainer. size/M Managed by automation to categorize the size of a PR. service/lambda Issues and PRs that pertain to the lambda service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. and removed size/M Managed by automation to categorize the size of a PR. labels Nov 19, 2019
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Nov 19, 2019
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed dependencies Used to indicate dependency changes. needs-triage Waiting for first response or review from a maintainer. labels Nov 19, 2019
@bflad bflad added this to the v2.39.0 milestone Nov 19, 2019
@stefansundin
Copy link
Contributor

Sorry for hijacking this PR, but I just wanted to say that I think this kind of validation seems kind of pointless.. or at least not very useful. We know that this list will change in the future, and do we really want to force people to wait a few days before they can use it? And folks who are stuck on old provider versions for various reasons will be unable to use the new runtime until they can get their stuff fixed.

This is not the only place where terraform does this, I have had this other PR open since July and haven't heard a word. I think this strategy should be rethought and validation that prevents an apply should be used sparingly. Worst case AWS will return an error, but at least terraform won't be the blocker.

How about instead of:

Error: expected runtime to be one of [dotnetcore1.0 dotnetcore2.0 dotnetcore2.1 go1.x java8 nodejs4.3 nodejs4.3-edge nodejs6.10 nodejs8.10 nodejs10.x provided python2.7 python3.6 python3.7 ruby2.5], got nodejs12.x

Terraform can say:

Warning: unexpected runtime nodejs12.x. Apply at your own risk.

Just a thought.. Hope you consider it!

@dfens1
Copy link

dfens1 commented Nov 20, 2019

I’d say the hard-coded check in terraform for this and many other cases could be made dynamic. If I set the runtime to nodejs13.x, terraform plan could already check against available runtimes (for my region) against my requested runtime. And in this case it could return an error “specified runtime not available”

@ewbankkit
Copy link
Contributor

N.B. This code change also affects the aws_lambda_layer_version resource and data source.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @ethanrubio 🚀

Output from acceptance testing:

--- PASS: TestAccAWSLambdaFunction_expectFilenameAndS3Attributes (21.16s)
--- PASS: TestAccAWSLambdaFunction_nilDeadLetterConfig (23.01s)
--- PASS: TestAccAWSLambdaFunction_versioned (39.63s)
--- PASS: TestAccAWSLambdaFunction_Layers (43.08s)
--- PASS: TestAccAWSLambdaFunction_basic (48.23s)
--- PASS: TestAccAWSLambdaFunction_updateRuntime (54.48s)
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfigUpdated (58.19s)
--- PASS: TestAccAWSLambdaFunction_EmptyVpcConfig (38.52s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_noRuntime (1.41s)
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfig (63.74s)
--- PASS: TestAccAWSLambdaFunction_versionedUpdate (66.19s)
--- PASS: TestAccAWSLambdaFunction_s3 (31.20s)
--- PASS: TestAccAWSLambdaFunction_KmsKeyArn_NoEnvironmentVariables (71.40s)
--- PASS: TestAccAWSLambdaFunction_concurrency (71.84s)
--- PASS: TestAccAWSLambdaFunction_concurrencyCycle (72.22s)
--- PASS: TestAccAWSLambdaFunction_localUpdate (35.43s)
--- PASS: TestAccAWSLambdaFunction_tracingConfig (78.68s)
--- PASS: TestAccAWSLambdaFunction_encryptedEnvVariables (79.60s)
--- PASS: TestAccAWSLambdaFunction_localUpdate_nameOnly (34.89s)
--- PASS: TestAccAWSLambdaFunction_envVariables (84.31s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_NodeJs810 (28.94s)
--- PASS: TestAccAWSLambdaFunction_s3Update_basic (37.60s)
--- PASS: TestAccAWSLambdaFunction_s3Update_unversioned (36.12s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_NodeJs10x (30.52s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_NodeJs12x (29.82s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python27 (27.62s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_java8 (28.04s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_java11 (28.11s)
--- PASS: TestAccAWSLambdaFunction_LayersUpdate (101.41s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_provided (27.35s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python36 (27.80s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python37 (27.67s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python38 (27.46s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_ruby25 (27.38s)
--- PASS: TestAccAWSLambdaFunction_tags (42.04s)
--- PASS: TestAccAWSLambdaFunction_VpcConfig_ProperIamDependencies (1374.52s)
--- PASS: TestAccAWSLambdaFunction_VPC (1550.25s)

@bflad bflad merged commit 01c94a8 into hashicorp:master Nov 20, 2019
@bflad
Copy link
Contributor

bflad commented Nov 20, 2019

Hi @stefansundin / @dfens1 to follow up on your comments, you may be interested in these two Terraform Plugin SDK enhancement issues:

I do not believe we have intentions of removing these validations as errors since they prevent real world issues, especially for newer operators or those unfamiliar with an API and its expected values. That said if you feel strongly about these topics, we would suggest opening a proposal issue for further discussion.

@ghost
Copy link

ghost commented Nov 21, 2019

This has been released in version 2.39.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

pixelpogo pushed a commit to babbel/terraform-provider-aws that referenced this pull request Nov 22, 2019
@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/lambda Issues and PRs that pertain to the lambda service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS Lambda support for Java 11, Node.js 12 and Python 3.8
5 participants