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

Allow specifying a retention policy for lambda layers #6010

Merged
merged 2 commits into from Apr 12, 2019
Merged

Conversation

@dschep
Copy link
Contributor

dschep commented Apr 10, 2019

What did you implement:

closes #5765 by adding an option to allow the retention of previous layer verions

How did you implement it:

If the user specifies retain: true for their layer, DeletionPolicy: Retain is added to the cloudformation template, and the sha of the layer CFN is appended to the logical ID (otherwise old layer versions are still deleted)

How can we verify it:

npm i -g serverless/serverless#retain-layer-versions
sls create -t aws-nodejs
echo 'layers: {foobar: {path: foobar, retain: true}}' >> serverless.yml
echo foo > foobar/foo
sls deploy
echo bar > foobar/bar
sls deploy

Then check that your lambda console has 2 versions of the foobar layer.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@dschep dschep requested review from pmuens and eahefnawy Apr 10, 2019
@pmuens pmuens added this to In progress in Serverless via automation Apr 10, 2019
@pmuens pmuens added this to the 1.41.0 milestone Apr 10, 2019
Serverless automation moved this from In progress to Reviewer approved Apr 11, 2019
Copy link
Member

pmuens left a comment

This looks good @dschep ! 👍

I just added some comments while reading through the code. Nothing important though...

@@ -36,6 +36,7 @@ layers:
licenseInfo: GPLv3 # optional, a string specifying license information
allowedAccounts: # optional, a list of AWS account IDs allowed to access this layer.
- '*'
retain: false # optional, false by default. If true, layer versions are not deleted as new ones are created

This comment has been minimized.

Copy link
@pmuens

pmuens Apr 11, 2019

Member

Should we also add this config to the serverless.yml.md file? (the root layers definition is completely missing right now).

Serverless automation moved this from Reviewer approved to Needs review Apr 12, 2019
@dschep dschep requested a review from pmuens Apr 12, 2019
Serverless automation moved this from Needs review to Reviewer approved Apr 12, 2019
@pmuens
pmuens approved these changes Apr 12, 2019
Copy link
Member

pmuens left a comment

Thanks for updating @dschep 👍

LGTM :shipit:

@@ -262,6 +262,18 @@ functions:
pool: MyUserPool
trigger: PreSignUp
layers:

This comment has been minimized.

Copy link
@pmuens

pmuens Apr 12, 2019

Member

🙏 thanks!

@dschep dschep merged commit ccb3576 into master Apr 12, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Serverless automation moved this from Reviewer approved to Done Apr 12, 2019
@dschep dschep deleted the retain-layer-versions branch Apr 12, 2019
@nford

This comment has been minimized.

Copy link

nford commented May 1, 2019

This breaks cross-stack references because it randomizes the logical id of the layer. I now have this in the generated CloudFormation:
"CommonLibsLambdaLayer251dd0d4097dfe2edcf097d2f29b90b5d4be13c5": { "Type": "AWS::Lambda::LayerVersion",...

@dschep

This comment has been minimized.

Copy link
Contributor Author

dschep commented May 1, 2019

@nford, That's necessary to work around cloudformation limitations. If it were to use the same logical id, the new version would replace the old one, thus the retain property would do nothing. There is however still an Output with a predictable name. Eg, foobar in the serverless.yml results in a output with the name FoobarLambdaLayerQualifiedArn

@nford

This comment has been minimized.

Copy link

nford commented May 1, 2019

I'll have to experiment with SAM to see if it too randomizes the logical id

@dschep

This comment has been minimized.

Copy link
Contributor Author

dschep commented May 1, 2019

iirc it does.

@nford

This comment has been minimized.

Copy link

nford commented May 2, 2019

@dschep How can I export the value of this output generated by the framework so it can be used in another stack? Where CloudFormation has output "CommonLibsLambdaLayerQualifiedArn" with the description "Current Lambda layer version" the following fails:

Outputs:
    commonLibsLambdaLayerArn:
        Value: !Ref CommonLibsLambdaLayerQualifiedArn
        Export:
            Name: commonLibsLambdaLayerArn

With error:

" The CloudFormation template is invalid: Unresolved resource dependencies [CommonLibsLambdaLayerQualifiedArn] in the Outputs block of the template"

I guess I'm sort of re-outputing an output, but there seems to be no way to export a layer with retain set to true, and there is no way to update a stack referencing a layer in another stack with retain set to false.

@nford

This comment has been minimized.

Copy link

nford commented May 2, 2019

Looks like this issue #3661 was opened to try to resolve this problem, but the issue was closed by @pmuens based on a suggested solution that myself and others can confirm does not work.

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented May 3, 2019

@nford You can't use !Ref in serverless.yml. You need to use Ref:. That should do the trick...

@nford

This comment has been minimized.

Copy link

nford commented May 3, 2019

@pmuens that's in the Outputs section - it's cloudformation syntax. Just changed it to:

Outputs:
    lambdaFunctionRoleArn:
        Value: !GetAtt lambdaFunctionRole.Arn
        Export:
            Name: ${self:service}-${opt:stage}-lambdaFunctionRoleArn
    commonLibsLambdaLayerArn:
        Value:
            Ref: CommonLibsLambdaLayerQualifiedArn
        Export:
            Name: commonLibsLambdaLayerArn

And the result is the same, all my other values export fine to the other stack but

The CloudFormation template is invalid: Unresolved resource dependencies [CommonLibsLambdaLayerQualifiedArn] in the Outputs block of the template

@nford

This comment has been minimized.

Copy link

nford commented May 6, 2019

@pmuens "That should do the trick..." did not do the trick.

Do you have a tested resolution for this issue or are you willing to reopen #3661 ?

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented May 7, 2019

@nford the ! directive is not supported so you need to update your usage of !GetAtt as well.

@nford

This comment has been minimized.

Copy link

nford commented May 7, 2019

@pmuens that is not the output that is causing the error, and that syntax DOES work in the outputs section of the template, here is a screenshot of my outputs https://imgur.com/a/ttPNuYu

Using the following Outputs code:

Outputs:
    apiGatewayId:
        Value: !Ref apiGateway
        Export:
            Name: ${self:service}-${opt:stage}-apiGatewayId
   apiGatewayRootResourceId:
      Value: !GetAtt apiGateway.RootResourceId
      Export:
          Name: ${self:service}-${opt:stage}-apiGatewayRootResourceId
   lambdaFunctionRoleArn:
       Value: !GetAtt lambdaFunctionRole.Arn
       Export:
           Name: ${self:service}-${opt:stage}-lambdaFunctionRoleArn

Notice CommonLibsLambdaLayerQualifiedArn is there but when I add it manually:

    commonLibsLambdaLayerArn:
         Value:
               Ref: CommonLibsLambdaLayerQualifiedArn
         Export:
               Name: commonLibsLambdaLayerArn

to outputs in an effort to add an EXPORT for it, I get the error

Anyway, thanks for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Serverless
  
Done
3 participants
You can’t perform that action at this time.