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

Add function artifact check in deployFunction #3856

Merged
merged 9 commits into from Jul 10, 2017

Conversation

@darkowlzz
Copy link
Contributor

commented Jun 26, 2017

What did you implement:

Closes #3625
Closes #3580

How did you implement it:

Added function artifact check in deployFunction() and skip packing when an artifact is provided.
And add function artifact check in packageService() to use the provided artifact path and not derive some default artifact path.

How can we verify it:

  • Create an aws-nodejs template project and deploy.
  • Create a dummy zip file, say foo.zip, and add that as artifact of a function.
functions:
  hello:
    handler: handler.hello
    package:
      artifact: foo.zip
  • Run sls deploy function -f hello to deploy the function.
  • Open AWS lambda console and export the function. The downloaded file is the same dummy zip file.

Todos:

  • Write tests
  • 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 Jun 26, 2017
@pmuens pmuens added this to the 1.17 milestone Jun 27, 2017
Copy link
Member

left a comment

Thanks for fixing this @darkowlzz 👍

This looks pretty promising. I've tested it with a couple of different setups and couldn't break it. However I'll test it more thoroughly the upcoming days.

I added a comment about a missing test case. Other than that I believe it should be GTM quite soon!

Let us know if you need any help here! 👍

.getFunctionArtifactName(this.options.function);
const artifactFilePath = path.join(this.packagePath, artifactFileName);
const functionObject = this.serverless.service.getFunction(this.options.function);
let artifactFilePath = functionObject.artifact;

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 27, 2017

Member

We should add a test for that

@darkowlzz darkowlzz force-pushed the darkowlzz:3625-deploy-function-pkg branch from a445be5 to a7e9aa9 Jun 28, 2017

return awsDeployFunction.deployFunction().then(() => {
// const expected = 'foo.zip';
// expect(fs.readFileSync.calledWithExactly(expected)).to.be.equal(true);

This comment has been minimized.

Copy link
@darkowlzz

darkowlzz Jun 28, 2017

Author Contributor

This test checks when package.artifact is provided, it's read by fs.readFileSync().

Copy link
Member

left a comment

Thanks for updating the PR @darkowlzz 👍

I just looked through the code and my assumption is that root cause of the weir behavior is that one of the tests here fails and the stubs are not resettet which means that e.g. readFileSync calls are still stubbed out.

I added some comments. Generally speaking all the stub usages should be moved into beforeEach() and afterEach() block.

You can find an example here:

beforeEach(() => {
globbySyncStub = sinon.stub(globby, 'sync');
processChdirStub = sinon.stub(process, 'chdir').returns();
execSyncStub = sinon.stub(childProcess, 'execSync');
});
afterEach(() => {
process.chdir.restore();
globby.sync.restore();
childProcess.execSync.restore();
});

That ensures that the stubs are always correctly setup and always restored correctly.

I have not tested it thoroughly yet. Will do this next up.

Let us know if you still face any other roadblocks 👍

@@ -139,5 +139,24 @@ describe('AwsDeployFunction', () => {
fs.statSync.restore();
});
});

it('should read the provided artifact', () => {
sinon.stub(fs, 'readFileSync').returns('some dummy data');

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 29, 2017

Member

All the stubs should be moved in a beforeEach() block and reset in an afterEach() block.

return awsDeployFunction.deployFunction().then(() => {
// const expected = 'foo.zip';
// expect(fs.readFileSync.calledWithExactly(expected)).to.be.equal(true);
awsDeployFunction.provider.request.restore();

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 29, 2017

Member

The restoring should happen in an afterEach() block.

@darkowlzz darkowlzz force-pushed the darkowlzz:3625-deploy-function-pkg branch 2 times, most recently from 6246825 to 8188b17 Jun 29, 2017
@darkowlzz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2017

@pmuens thank you. You were right. Putting the stubbing and restoring into beforeEach and afterEach solved the issue 🙌

@pmuens

This comment has been minimized.

Copy link
Member

commented Jun 29, 2017

@pmuens thank you. You were right. Putting the stubbing and restoring into beforeEach and afterEach solved the issue 🙌

Awesome! Thanks for updating @darkowlzz 👍

Next up we'll take this for another spin.

@serverless serverless deleted a comment from darkowlzz Jun 30, 2017
@serverless serverless deleted a comment from darkowlzz Jun 30, 2017
@serverless serverless deleted a comment from darkowlzz Jun 30, 2017
@pmuens pmuens added pr/in-review and removed pr/in-progress labels Jun 30, 2017
Copy link
Member

left a comment

Hey @darkowlzz thanks for updating this PR again 👍
Really appreciate your work on this bug fix!

I just took a deep dive today and added some more comments to the PR.

Unfortunately I was able to break it with the following serverless.yml config when running serverless deploy (which works in master):

service:
  name: test-service

provider:
  name: aws
  runtime: nodejs6.10

package:
  individually: true

functions:
  hello:
    handler: handler.hello
    package:
      artifact: artifacts/hello.zip

@eahefnawy could you also take a deep dive into this PR and test all the possible edge cases?

Thanks again @darkowlzz 👍

const functionObject = this.serverless.service.getFunction(this.options.function);
const funcPackageConfig = functionObject.package || {};
let artifactFilePath = funcPackageConfig.artifact;
// if function artifact is not provided, derive the default artifact path

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 30, 2017

Member

I would suggest to switch this statement and assume that the user is not using an artifact at the function package level (since most of the time this would be the case).


beforeEach(() => {
fsReadFileStub = sinon.stub(fs, 'readFileSync').returns('some dummy data');
sinon.stub(fs, 'statSync').returns({ size: 2048 });

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 30, 2017

Member

We should add an expect that this stub was also called.

beforeEach(() => {
fsReadFileStub = sinon.stub(fs, 'readFileSync').returns('some dummy data');
sinon.stub(fs, 'statSync').returns({ size: 2048 });
sinon.stub(serverless.service, 'getFunction').returns({

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 30, 2017

Member

We should add an expect that this stub was also called.

artifact: 'foo.zip',
},
});
sinon.stub(awsDeployFunction.provider, 'request').resolves();

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 30, 2017

Member

We should add an expect that this stub was also called.

const exclude = this.getExcludes(funcPackageConfig.exclude);
const include = this.getIncludes(funcPackageConfig.include);
const zipFileName = `${functionName}.zip`;

return this.zipService(exclude, include, zipFileName).then(artifactPath => {
functionObject.artifact = artifactPath;

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 30, 2017

Member

Could you give a little more context why this change is needed?

This comment has been minimized.

Copy link
@darkowlzz

darkowlzz Jun 30, 2017

Author Contributor

I found that function object returned by serverless.service.getFunction() has artifact inside package, similar to how we specify them in serverless.yaml,

functions:
  hello:
    handler: handler.hello
    package:
      artifact: artifactName.zip

Along with exclude and include. Is that not the case?

darkowlzz added 4 commits Jun 26, 2017
- Adds check for function package artifact in packageFunction() and
avoids packing if a package artifact is found.
- Adds check for function artifact in deployFunction() and use the
provided artifact to deploy if available.
@darkowlzz darkowlzz force-pushed the darkowlzz:3625-deploy-function-pkg branch from 8188b17 to d5de23f Jun 30, 2017
@darkowlzz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

Unfortunately I was able to break it with the following serverless.yml config when running serverless deploy (which works in master):

@pmuens that was happening because of how the extendedValidate() was getting artifact name and path. Changed that to check function object for artifact. Seems to work now. 🙂

pmuens added 4 commits Jul 3, 2017
@pmuens
pmuens approved these changes Jul 4, 2017
Copy link
Member

left a comment

Thanks for all the work you've put into this @darkowlzz 👍 💯

I just pushed some minor changes regarding tests to this PR and reviewed / tested it thoroughly this morning.

Wasn't able to break it. Here's the service which I used (I un/commented different parts during the tests):

service:
  name: test-service

provider:
  name: aws
  runtime: nodejs6.10

package:
  individually: true

functions:
  hello:
    handler: handler.hello
    package:
      artifact: artifacts/hello.zip
  goodbye:
    handler: handler.goodbye
    package:
      artifact: artifacts/goodbye.zip

GTM from my side :shipit:

Leaving the stage for @eahefnawy to test and merge it if he won't find anything else 👍 🎉

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

@darkowlzz @pmuens thanks for that. I've also took a deep dive and test it with the following edge cases, with one unfortunate failed test:

  • sls deploy with packaging individually
  • sls deploy with packaging with package path
  • sls deploy with packaging with artifact at function level
  • sls deploy with packaging with artifact at service level
  • sls deploy function with packaging individually
  • sls deploy functon with packaging with package path
  • sls deploy function with packaging with artifact at function level
  • sls deploy function with packaging with artifact at service level.

@pmuens could you try to reproduce? with the following config:

package:
  artifact: dummy.zip # just zip any file you have on your machine and put it in your service

functions:
  hello:
    handler: handler.hello
  • run sls deploy,
  • go to AWS and export/download the hello function zip and inspect the contents, you'll find that the content you zipped exists rather than the default service contents, which is what is desired.
  • run sls deploy function --hello
  • go to AWS and export/download the hello function zip and inspect the contents, you'll find that the content you zipped is actually the service default, which is not cool because we've specified a zip file at the service level to be used by all functions without an explicit artifact
@pmuens

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

@eahefnawy thanks for the in-depth test and the list with the different cases you've tested against.

I was able to reproduce the issue. However this issue is also in master so it seems like it's unrelated to this PR.

@eahefnawy could you try to reproduce the issue in master and confirm that it's not related to this PR?

@eahefnawy eahefnawy modified the milestones: 1.18, 1.17 Jul 5, 2017
@eahefnawy

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

@darkowlzz @pmuens I just fixed this issue and opened another follow up PR based on this PR. Thanks a lot for your efforts here @darkowlzz, they're all still included in the follow up PR 😊

@pmuens

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

@darkowlzz @pmuens I just fixed this issue and opened another follow up PR based on this PR. Thanks a lot for your efforts here @darkowlzz, they're all still included in the follow up PR 😊

Great! Thanks for that @eahefnawy 👍

Should we close this one?

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

@pmuens since all the commits here are included in my PR, I think Github will mark this PR as merged too once my PR is merged. So let's just leave it open and let Github do the git magic

@pmuens

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

@pmuens since all the commits here are included in my PR, I think Github will mark this PR as merged too once my PR is merged. So let's just leave it open and let Github do the git magic

Alright. That makes sense! Sounds like a good plan 👍

@eahefnawy eahefnawy merged commit e2df8f2 into serverless:master Jul 10, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eahefnawy added a commit that referenced this pull request Jul 10, 2017
…on-pkg

Follow up for #3876 & #3856: Fixes for custom package artifacts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.