search

leftclickben / serverless-api-stage

AWS API Gateway stage plugin for serverless framework
MIT License
47 stars 17 forks source link

MethodSettings should be a list #8

Closed e0ipso closed 6 years ago

e0ipso commented 6 years ago

Thanks again for the plugins.

According to the docs MethodSettings should be an array of objects. However the plugin only takes a single object and makes it into an array of one position.

This PR will allow specifying multiple method settings while maintaining backwards compatibility for the existing installs.

leftclickben commented 6 years ago

@e0ipso the changes look sensible, but I'm unclear as to why you need to pass multiple MethodSettings? The stage can only have one lot of settings so how does this work, won't it just merge them all anyway? Or am I missing something? I've never tried sending more than one object in that array...

e0ipso commented 6 years ago

@leftclickben you can use either method in your serverless.yml. The current:

…
    MethodSettings:
      LoggingLevel: INFO
      CachingEnabled: true
      CacheTtlInSeconds: 3600

Or the new:

    MethodSettings:
      - LoggingLevel: INFO
        CachingEnabled: true
        HttpMethod: GET,
        CacheTtlInSeconds: 60,
        ResourcePath: `/~1foo~1bar`, # GET /foo/bar
      - LoggingLevel: INFO
        CachingEnabled: true
        HttpMethod: POST,
        CacheTtlInSeconds: 300,
        ResourcePath: `/~1lorem~1ipsum`, # POST /lorem/ipsum
leftclickben commented 6 years ago

Ah yes of course, per-resource settings, got it.

leftclickben commented 6 years ago

@e0ipso thing missing then is a test that demonstrates this, can you please add one? Then I'll be happy to merge this. Otherwise I can write the test but I'm not sure when I will get a chance to.

leftclickben commented 6 years ago

Sorry, that was meant to read, "the only thing missing is a test..."

leftclickben commented 6 years ago

@e0ipso and thanks for the contributions 👍

e0ipso commented 6 years ago

thing missing then is a test that demonstrates this

I thought about that back in the day. However, I think that the current test still being green (still getting cast to an array) is proof enough. More tests than that only will test that Array.prototype.concat works as expected, which we should not need to test.

e0ipso commented 6 years ago

@e0ipso and thanks for the contributions 👍

Thanks to this your code is my code. That means it's also my responsibility to contribute to it ;-)

leftclickben commented 6 years ago

More tests than that only will test that Array.prototype.concat works as expected

Well, it would test the loop that you added, to ensure that multiple records in results in multiple records out. But yes I agree the benefit is fairly nebulous. I'll merge this and if I feel like adding tests later, I can.

That means it's also my responsibility to contribute to it

I guess I was thanking you on behalf of anyone using this code :-)