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

Ensure that changes to logs.restApi settings are propagated to all methods #99

Closed
wants to merge 1 commit into from

Conversation

driskell
Copy link

Overview

If you use this plugin then after deploying attempt to change logging settings in serverless.yml, then it will not update, because all methods have overridden settings from the stage settings and serverless will only update the default settings.

Proposed Fix

As well as updating caching settings, copy the logs settings from */* to ensure that any changes to them are maintained and applied at the method level. By copying from */* we also ensure that a serverless.yml using a shared API Gateway can copy the settings properly even though they are defined elsewhere in another serverless.yml (or defined via Terraform or otherwise.)

@driskell
Copy link
Author

I can try and work on tests if this is an acceptable approach.

@DianaIonita
Copy link
Owner

Hi @driskell,

Thank you for this fix! Yes, I think this is a good approach.
If you'd like to have a go at a test fixture, it's probably easiest to start out with a new test file, just for checking that stage settings are propagated. You'll probably want to mock the call to get the stage via the serverless object. An example of mocking one of the aws methods is here. That Serverless class was created specifically for tests.

Please let me know if you need any help!

@DianaIonita
Copy link
Owner

Hi @driskell,

I included your changes along with tests, a toggle and associated documentation in this PR.
Thanks for your help!

@DianaIonita DianaIonita closed this Dec 7, 2022
@driskell
Copy link
Author

driskell commented Dec 7, 2022

Hi @driskell,

I included your changes along with tests, a toggle and associated documentation in this PR. Thanks for your help!

Thanks! I can’t see any attribution in the commits though such as co-authors, was it missed?

@DianaIonita
Copy link
Owner

Thanks! I can’t see any attribution in the commits though such as co-authors, was it missed?

Hi @driskell,

Sorry about that, it was simpler to add the extra setting and tests on my own branch then just merge it into develop as a whole feature. I did credit you in the release notes and I have just updated my PR to reference yours. I hope that's alright 😊

@driskell
Copy link
Author

Thanks! I can’t see any attribution in the commits though such as co-authors, was it missed?

Hi @driskell,

Sorry about that, it was simpler to add the extra setting and tests on my own branch then just merge it into develop as a whole feature. I did credit you in the release notes and I have just updated my PR to reference yours. I hope that's alright 😊

No worries! 👍 Just usually I see things cherry picked then commits added so all authors are carried over (and I think they squash properly too) and it then bakes it into the Git history. All good and thanks for all your hard work on this project 😄

@driskell driskell deleted the patch-1 branch December 21, 2022 11:59
@DianaIonita
Copy link
Owner

Got it, thanks, I'll remember for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants