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

New Data Source: aws_lambda_invocation #4222

Merged
merged 7 commits into from
May 14, 2018

Conversation

spirius
Copy link
Contributor

@spirius spirius commented Apr 16, 2018

Data source for using lambda function as data provider, similar to External Data Source.

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 16, 2018
@spirius spirius force-pushed the feature/lambda-invoke branch from 6b1fc05 to 4d2952b Compare April 16, 2018 16:39
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 16, 2018
@bflad bflad added new-data-source Introduces a new data source. service/lambda Issues and PRs that pertain to the lambda service. labels Apr 17, 2018
@spirius spirius force-pushed the feature/lambda-invoke branch from 4d2952b to 1e09e14 Compare April 17, 2018 19:57
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@spirius
Copy link
Contributor Author

spirius commented Apr 17, 2018

Test results

$ TF_ACC=1 go test -v -run=TestAccDataSourceAwsLambdaInvoke_* -timeout 120m
=== RUN   TestAccDataSourceAwsLambdaInvoke_basic
--- PASS: TestAccDataSourceAwsLambdaInvoke_basic (51.64s)
=== RUN   TestAccDataSourceAwsLambdaInvoke_qualifier
--- PASS: TestAccDataSourceAwsLambdaInvoke_qualifier (50.15s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws   101.834s

@spirius spirius changed the title [WIP] New Data Source: aws_lambda_invoke New Data Source: aws_lambda_invoke Apr 17, 2018
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.

Hi @spirius! Thanks for submitting this! I left some initial comments below. Can you please take a look and let us know if you have any questions or do not have time to implement the feedback? 👍

"github.com/hashicorp/terraform/helper/schema"
)

func dataSourceAwsLambdaInvoke() *schema.Resource {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please rename all the relevant functions to dataSourceAwsLambdaFunctionInvocation? Thanks!

Default: "$LATEST",
},

"input": {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make this TypeString, which will allow us to use ValidateFunc: validateJsonString, and remove JSON marshal handling/errors from the data source.

},
},

"result": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here -- we should convert this to TypeString and remove the JSON unmarshal handling/errors from the data source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use TypeString for result, how the returned data will be used later in terraform code? As far as I know there is no jsondecode or similar function available.


res, err := conn.Invoke(&lambda.InvokeInput{
FunctionName: aws.String(functionName),
InvocationType: aws.String(lambda.InvocationTypeRequestResponse),
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, we can allow a new invocation_type attribute for calling Lambda functions asynchronously with Event 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, I wanted to keep the first version simple, but I think we can pass some random s3 key to lambda during invocation and expect to get result stored there.

aws/provider.go Outdated
@@ -211,6 +211,7 @@ func Provider() terraform.ResourceProvider {
"aws_kms_ciphertext": dataSourceAwsKmsCiphertext(),
"aws_kms_key": dataSourceAwsKmsKey(),
"aws_kms_secret": dataSourceAwsKmsSecret(),
"aws_lambda_invoke": dataSourceAwsLambdaInvoke(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please rename the data source aws_lambda_function_invocation? Thanks!

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 25, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 28, 2018
@spirius spirius changed the title New Data Source: aws_lambda_invoke New Data Source: aws_lambda_invocation Apr 28, 2018
@PauloMigAlmeida
Copy link

@spirius Apparently, there are conflicts with the master branch that need to be solved before anyone from Hashicorp can take further action on this. Would you mind addressing them?

@bflad Apart from these conflicts, what else needs to be done in order to get this approved/merged into master? I'm happy to contribute to it in any way possible.

Cheers!

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label May 11, 2018
@spirius
Copy link
Contributor Author

spirius commented May 11, 2018

@PauloMigAlmeida conflict fixed.

@bflad I've updated code according to your comments. The only question I have is regarding result field. If we use TypeString for it, how the result can be used later in terraform since it needs to be unmarshalled somehow and there is no builtin function in terraform for doing that.

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label May 11, 2018
@bflad
Copy link
Contributor

bflad commented May 11, 2018

For handling the response, what do you think about the below? 😄

  • Renaming the existing result attribute to result_map
  • Creating a new result attribute using TypeString with the "full" JSON string
  • Ensuring the logic is setup so it does not completely fail when the JSON cannot be unmarshalled into the map[string]interface{} (and document the behavior that the attribute will show as missing downstream if the JSON cannot be unmarshalled)

This enables these use cases:

  • Advanced operators that are comfortable with using null_resource or one of the community "JSON decoding" providers, can utilize the TypeString attribute today
  • When jsondecode is implemented as an interpolation function upstream in Terraform core, people can immediately use the TypeString attribute without AWS provider code changes. The jsondecode function is being tracked upstream at: jsondecode function terraform#10363
  • Operators can still use the limited TypeMap implementation today, if their Lambda response fits the mold

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label May 14, 2018
@spirius
Copy link
Contributor Author

spirius commented May 14, 2018

Hi @bflad, renamed result to result_map and added raw result to data source.

Test results:

$ TF_ACC=1 go test -v -run=TestAccDataSourceAwsLambdaInvocation_* -timeout 120m ./aws
=== RUN   TestAccDataSourceAwsLambdaInvocation_basic
--- PASS: TestAccDataSourceAwsLambdaInvocation_basic (51.32s)
=== RUN   TestAccDataSourceAwsLambdaInvocation_qualifier
--- PASS: TestAccDataSourceAwsLambdaInvocation_qualifier (53.58s)
=== RUN   TestAccDataSourceAwsLambdaInvocation_complex
--- PASS: TestAccDataSourceAwsLambdaInvocation_complex (50.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	155.076s

@bflad bflad added this to the v1.19.0 milestone May 14, 2018
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.

Thanks for working through this @spirius! Looks great, I just needed to randomize the test resource naming so the testing could work in our parallelized acceptance testing environment.

make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsLambdaInvocation'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsLambdaInvocation -timeout 120m
=== RUN   TestAccDataSourceAwsLambdaInvocation_basic
--- PASS: TestAccDataSourceAwsLambdaInvocation_basic (39.70s)
=== RUN   TestAccDataSourceAwsLambdaInvocation_qualifier
--- PASS: TestAccDataSourceAwsLambdaInvocation_qualifier (38.59s)
=== RUN   TestAccDataSourceAwsLambdaInvocation_complex
--- PASS: TestAccDataSourceAwsLambdaInvocation_complex (38.83s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	117.159s

@bflad bflad merged commit 7c73567 into hashicorp:master May 14, 2018
bflad added a commit that referenced this pull request May 14, 2018
@spirius spirius deleted the feature/lambda-invoke branch May 15, 2018 14:09
@bflad
Copy link
Contributor

bflad commented May 17, 2018

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

@ghost
Copy link

ghost commented Apr 6, 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 Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-data-source Introduces a new data source. service/lambda Issues and PRs that pertain to the lambda service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants