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.

Sign up for GitHub

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

Jump to bottom

Add --update-config flag to deploy function command #4173

Merged
merged 12 commits into from
Sep 8, 2017
Merged

Add --update-config flag to deploy function command #4173

merged 12 commits into from
Sep 8, 2017

Conversation

RafalWilinski
Copy link
Contributor

@RafalWilinski RafalWilinski commented Aug 29, 2017 •
edited
Loading

What did you implement:

Closes #4042

As issue suggests, this PR uses only UpdateFunctionConfiguration, so it does zero changes to CloudFormation template.

How can we verify it:

Deploy simple service:

service: aws-nodejs-tests

provider:
  name: aws
  runtime: nodejs6.10
  stage: dev

functions:
  hello:
    handler: handler.hello
    name: ${self:provider.stage}-lambdaName

And then change some config values & run serverless deploy function -f hello -u

service: aws-nodejs-tests

provider:
  name: aws
  runtime: nodejs6.10
  stage: dev

functions:
  hello:
    handler: handler.hello
    name: ${self:provider.stage}-lambdaName
    description: Updated desc
    memorySize: 256
    timeout: 6
    environment:
      VARIABLE: varrrr
      VARTWO: two

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

@pmuens pmuens self-requested a review August 29, 2017 15:49
@eahefnawy
Copy link
Contributor

Works like a charm!! Thanks tons @RafalWilinski ! your contributions are priceless 😊

I'm thinking maybe update config should happen by default on sls deploy function, what you think? cc @pmuens

pmuens
pmuens suggested changes Sep 7, 2017
View reviewed changes
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Great stuff @RafalWilinski (as usual!) 👍. You really rock 🎸 🤘.

I just looked through the code and added some comments.

I'm thinking maybe update config should happen by default on sls deploy function, what you think? cc @pmuens

IMHO that would be super nice! Great idea @eahefnawy. This would make the whole functionality a lot easier and IMHO is exactly what I'd expect if I run serverless function deploy.

lib/plugins/aws/deployFunction/index.js Outdated
@@ -5,6 +5,7 @@ const _ = require('lodash');
const crypto = require('crypto');
const path = require('path');
const fs = require('fs');
const omitEmpty = require('omit-empty');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use https://lodash.com/docs/4.17.4#omit for that?

package.json Outdated
@@ -113,6 +113,7 @@
"minimist": "^1.2.0",
"moment": "^2.13.0",
"node-fetch": "^1.6.0",
"omit-empty": "^0.4.1",
Copy link
Contributor

@pmuens pmuens Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can't we use _.omit from lodash for that? This way we'd save one external dependency.

Copy link
Contributor Author

@RafalWilinski RafalWilinski Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, from my experience _.omit does not work for nested/deep values.

lib/plugins/aws/deployFunction/index.js Outdated
@@ -66,6 +72,38 @@ class AwsDeployFunction {
});
}

updateFunctionConfiguration() {
const params = omitEmpty({
Copy link
Contributor

@pmuens pmuens Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Does this include config which is defined on a the provider level as well?

See: https://serverless.com/framework/docs/providers/aws/guide/serverless.yml/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

docs/providers/aws/guide/deploying.md Outdated
@@ -78,7 +78,7 @@ This deployment method does not touch your AWS CloudFormation Stack. Instead, i
serverless deploy function --function myFunction
```

**Note:** You can always enforce a deployment using the `--force` option.
**Note:** You can always enforce a deployment using the `--force` option or `--update-config` to change function configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a separate **Note:** section below that since the --force and --update-config flags provide two distinct functionalities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@eahefnawy
Copy link
Contributor

Alright! thanks for the feedback @pmuens 😊 ... Let's do that then @RafalWilinski 👍

@RafalWilinski
Copy link
Contributor Author

RafalWilinski commented Sep 7, 2017 •
edited
Loading

@pmuens @eahefnawy

  • removed omit-empty dependency
  • changed updateFunctionConfiguration behavior to take service and provider into account
  • changed --update-config flag behavior. By default, serverless deploy function will also update Lambda config while --update-config will just update config without deploying new code
  • made Coveralls happy again 😁

@pmuens pmuens added this to the 1.22 milestone Sep 8, 2017
pmuens
pmuens approved these changes Sep 8, 2017
View reviewed changes
Copy link
Contributor

@pmuens pmuens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you very much for this PR and the updates @RafalWilinski 👍

I just tested it today and it's working great! GTM --> :shipit:

@pmuens pmuens merged commit 0e51b03 into serverless:master Sep 8, 2017
@eahefnawy eahefnawy mentioned this pull request Sep 13, 2017
Merged
@RafalWilinski RafalWilinski deleted the 4042-update-lambda-cfg-on-deploy branch September 14, 2017 15:28
@estyh estyh mentioned this pull request Feb 22, 2018
Closed
@keithburrows keithburrows mentioned this pull request Nov 30, 2018
Closed
@bsarazin bsarazin mentioned this pull request Mar 8, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmuens pmuens pmuens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.22
Development

Successfully merging this pull request may close these issues.
3 participants
@RafalWilinski @eahefnawy @pmuens