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

Exclude node_modules completely if deploying to google cloud functions, as GCF installs package.json. #264

Merged
merged 4 commits into from Oct 31, 2017

Conversation

Projects
None yet
2 participants
@debanjanbasu
Copy link
Contributor

debanjanbasu commented Oct 30, 2017

Exclude node_modules completely if deploying to google cloud functions, as GCF installs package.json.

Added a check to see if the provider is Google

How can we verify it:

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

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Oct 30, 2017

Hi @debanjanbasu , thank you for the PR. Can you fill out the description template completely - especially the checkboxes and check "enable edits from maintainers" in your PR?

@HyperBrain
Copy link
Member

HyperBrain left a comment

Only a small semantical change :)

});
// Copy modules only if not google-cloud-functions
// GCF Auto installs the package json
if (_.get(this.serverless, 'service.provider.name') !== 'google') {

This comment has been minimized.

@HyperBrain

HyperBrain Oct 30, 2017

Member

I would prefer an early exit here if the provider is google instead of putting the default case into the if.

if (_.get(this.serverless, 'service.provider.name') === 'google') {
  return BbPromise.resolve();
}
...

This comment has been minimized.

@HyperBrain

HyperBrain Oct 31, 2017

Member

Updated this. I will add unit tests for the new behavior to restore coverage.

@HyperBrain HyperBrain merged commit dbceb1b into serverless-heaven:master Oct 31, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 91.083%
Details
@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Nov 1, 2017

@debanjanbasu May I ask you for a favor? There is still the column for GCF missing in the provider compatibility chart (README). Would you mind to prepare a PR that adds the GCF status to the README?
I do not use Google, so I'm not able to check whether it works or not 😄
The main task for that is #128

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment