silvermine / serverless-plugin-cloudfront-lambda-edge

Adds Lambda@Edge support to Serverless
MIT License
296 stars 41 forks source link

Added IncludeBody #50

Closed Krolcom closed 3 years ago

Krolcom commented 4 years ago

functions: lamdaFunctionName:

this is formatted as .

handler: handler.handler
memorySize: 128
timeout: 2
lambdaAtEdge:
  distribution: 'WebsiteDistribution'
  eventType: 'origin-request'
  **includeBody: true**
coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.4%) to 9.821% when pulling 4d866fbc4f2d604432a771740a8d2006a7523caf on Krolcom:master into faf72d88bbaa64cc50f4a40b051b7b0055fd324c on silvermine:master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.4%) to 9.821% when pulling 2e475145d049866780a92884244b8eee79f762d0 on Krolcom:master into d2860cbc9e209a38d87d4fd96f56440356b9e3ca on silvermine:master.

LinguineCode commented 3 years ago

Hey @Krolcom can you fix the coverage error so we can get this merged?

jthomerson commented 3 years ago

@solsglasses I fixed up @Krolcom's commits and added the necessary documentation. I am unable to test it at this time. If you're able to test that it works both with and without the includeBody param, let me know and I can merge this for you.

bryanvaz commented 3 years ago

Hey quick question @solsglasses, what's blocking the merge? Do we still need to bump up the coverage in order to merge?

david-gettins commented 3 years ago

Any movement on this?

Maybe I'm missing something but the current tests assert nothing anyway so why is coverage blocking? I understand from the contribution guide that you are trying to maintain 100% coverage but given the only test I can easily find in the repo asserts 1 equals 1 what good is the coverage restriction anyway?

describe('serverless-plugin-cloudfront-lambda-edge', function() {
   var plugin; // eslint-disable-line no-unused-vars

   beforeEach(function() {
      plugin = new Plugin(stubServerless(), {});
   });

   describe('TODO', function() {

      it('needs to be tested', function() {
         expect(1).to.eql(1);
      });

   });

});

Please can you point me towards some meaningful tests? I'd be happy to contribute to this to get it moving as this is a feature I require.

jthomerson commented 3 years ago

@david-gettins @Krolcom @bryanvaz @solsglasses it's not about the tests. It's just been about lack of time to test it. I'd asked above if someone could test the fixed-up branch with and without the parameter and see if it works. If it does, I'll merge it. If any of you can confirm that you've tested this branch both with and without the parameter and it works, I'm happy to merge.

jthomerson commented 3 years ago

@Krolcom @bryanvaz @solsglasses @david-gettins sorry, I see the confusion now. By "test it" I just mean can you point a project at this branch and actually run a deploy with it - not automated testing. Sorry for the confusion.

david-gettins commented 3 years ago

No problem. I understand, I've tried it with both true for viewer request and origin request and not included the value for both viewer response and origin response. Both have worked fine. I haven't tried any other configurations yet but I will.

jthomerson commented 3 years ago

Thanks @david-gettins - since I'm here working right now and don't want to let it slip again, I'll merge it. If you do find any bugs in some other combo, let us know, or send a PR. Thanks!

jthomerson commented 3 years ago

@Krolcom @solsglasses @david-gettins @bryanvaz I just cut v 2.2.0 that includes this. Thanks @Krolcom for the contribution, and @david-gettins for the testing!