Fix: check updated s3 code for aws_lambda_function

[alexef]

See #6794 for more details.

Disclaimer: this is my first Go PR, I don't have any prior experience with it. Ran make to test my changes.

[apparentlymart]

Do you think we should set these even if they haven't changed?

I'm thinking about the case where only s3_object_version has changed and s3_bucket and s3_key are still the same... in that case, I think we'd try to send a request with an empty S3Bucket and S3Key, right?

I think it might work for us to separate the "has it changed?" logic from the construction of the object. So as pseudo-code it would be like:

if any of s3_bucket, s3_key, s3_object_version, filename or source_code_hash have changed {
    codeReq = &lambda.UpdateFunctionCodeInput{
        FunctionName: aws.String(d.Id()),
    }
    if filename is set {
        codeReq.ZipFile = loadFileContent(v.(string))
    }
    else {
        codeReq.S3Bucket = d.Get("s3_bucket")
        codeReq.S3Key = d.Get("s3_key")
        if version is set {
            codeReq.S3ObjectVersion = d.Get("s3_object_Version")
        }
    }
    call UpdateFunctionCode with codeReq, handle errors
    call setPartial for all the code-related attributes
}

Do you think that makes sense? I think this way we'll always send S3Bucket and S3Key if there have been any changes to S3 attributes.

[alexef]

Yes, you're right. IMO s3_bucket and s3_key must be set for every request, even though they haven't changed.

Your pseudocode is more readable than my code, I will update the PR to reflect it.

[alexef]

@apparentlymart I've updated the code, but looking at it, shouldn't the next following lines be updated as well? (configUpdate).

I'll be afk for two weeks, please feel free to do anything to it.

[apparentlymart]

Thanks @alexef! I'll take a closer look at this soon.

[alexef]

I'm back. Let me know if I can help w/ anything :)

[alexef]

Rebased against master.

[apparentlymart]

Hi @alexef... sorry for the silence here. I've had to take a little break for Terraform work to deal with some "real work" at work but I do intend to come back and look at this again at some point, unless someone else beats me to it.

[alexef]

No worries. I'll keep pinging from time to time :)

[apparentlymart]

Hi @alexef! Sorry for the delay here. I took a look at this today.

The logic here seems plausible, but when I tried to run the acceptance tests I ran into some issues:

• The new test is looking for a function called tf_acc_lambda_name_s3, but the function that's created is actually called tf_acc_lambda_name_s3_unversioned.
• After I tweaked the tests to address that, I got a different error that wasn't so easy to explain:

    testing.go:265: Step 1 error: Expected a non-empty plan, but got an empty plan!

It seems like something's amiss in the test steps that's causing no diff to be detected on the second step, but I wasn't able to spot it after trying a few things. Maybe you can spot it?

The other thing I noticed (while trying to debug the problem above) is that it seems that on the update step it creates a new S3 object lambda-func-modified.zip rather than simply updating the original one lambda-func.zip in place. Perhaps it would be better to preserve the key name here and test that we're able to trigger an update based on the source_code_hash argument.

Sorry again for the delay. Please let me know if anything isn't clear here!

[alexef]

@apparentlymart I'm not sure how should I continue with this PR. At the time I wrote it, the change was trivial and the tests passed.

It's likely that the tests were broken when I rebased this PR from master. I'll have a second look at the code, but lacking the time/any experience with Go, I cannot guarantee I can fix it :(

[apparentlymart]

Hi again @alexef! Thanks for taking the time to update this.

I found that the acceptance tests were not working due to some mismatches between function name in the test config vs the test checks, but the fix was easy and so I just fixed it and merged this rebased version of your changes at 7c4a3bf.

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.