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

Fix service.provider.region resolution #6317

Merged
merged 6 commits into from Jun 28, 2019

Conversation

Projects
2 participants
@medikoo
Copy link
Member

commented Jun 28, 2019

Fixes regression introduced with #6200

After which resolution of ${self:provider.region} variable, resolved with undefined in cases where no region was provided in serverless.yml.

Fixes #6313

Additionally improved setup of integration package tests:

  • Separated into two test files:
    • lambda-files.tests.js tests content of generated lambda archives, on whether they contain expected files
    • cloudformation.tests.js - confirms on generated CF template
    • Improved organization of fixtures

Having that, added test that covers this case

Is this ready for review?: YES
Is it a breaking change?: NO

medikoo added some commits Jun 28, 2019

@medikoo medikoo self-assigned this Jun 28, 2019

@medikoo medikoo requested review from dschep and pmuens Jun 28, 2019

@medikoo medikoo added pr/in-review and removed pr/in-progress labels Jun 28, 2019

@medikoo medikoo added this to In progress in Serverless via automation Jun 28, 2019

@medikoo medikoo added this to the 1.47.0 milestone Jun 28, 2019

Serverless automation moved this from In progress to Reviewer approved Jun 28, 2019

@pmuens

pmuens approved these changes Jun 28, 2019

Copy link
Member

left a comment

I just tested it and it works, so it's GTM :shipit:

TBH I'm on the fence here. Usually it should be undefined since the idea behind ${self:} was to reference the serverless.yml file. If it's not defined, it's undefined. Looks like the meaning of ${self:} was extended in ways it was never meant to be. Maybe the reason this worked in the first place was even a mistake and was caused by another bug.

We can merge it, but it's quite inconsistent.

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

We can merge it, but it's quite inconsistent.

Yes I agree. Indeed documentation clearly states that: https://serverless.com/framework/docs/providers/aws/guide/variables#reference-properties-in-serverlessyml

But it also means that internally we play not clean. As it's not the only property which we add to self (e.g. it's same case with stage, variableSyntax and some others added during processing).
Ideally (if we follow doc) it should just resemble serlverless.yml configuration and nothing more should be there.

It appears some users for a long time relied on this side effects, and I thought it might be a good call to not change that with non breaking release. What do you think?

And in one of the next majors (or breaking release) I think we should switch to some more consistent handling.

I think best would be if we expose some uniform serverless/service object that's also advertised as single source of true (so users wouldn't have to do ${opt:region, self:provider.region} but can do just ${sls.provider.region}, something close to this proposal: #6031)

@pmuens

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Yes, I totally agree with your comments. AFAIK A LOT of users rely on this side-effect, so we cannot remove it. Let's rethink how this works once we can do breaking changes in the near future. For the time-being let's move on and merge this fix 👍

@medikoo medikoo merged commit 4a7e1b3 into master Jun 28, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (medikoo) No manifest changes detected

Serverless automation moved this from Reviewer approved to Done Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.