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

Improve Stage and Region Usage #4560

Merged
merged 2 commits into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@erikerikson
Member

erikerikson commented Dec 12, 2017

What did you implement:

Centralize throughout the codebase to using the aws provider's getStage and getRegion methods.

Remove the errant (but understandable) distributed usage of region and stage settings. This otherwise locks in a multitude of bugs around the improper algorithm for selecting (given all context) the proper region or stage setting. Instead, all code should use the centralized algorithm for determining such values. This creates a strange first and second class configuration concept but these two are sufficiently varied and complex in their creation and use that this seems appropriate.

How did you implement it:

Centralize throughout the codebase to using the aws provider's getStage and getRegion methods.

How can we verify it:

verify by noting the tests continue to pass.

is there something more extensive than the unit tests that I should run in the future to verify across the cli interactions or the like?

Todos:

  • Write tests
  • Write documentation
  • 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

Improve Stage and Region Usage
Remove the errant (but understandable) distributed usage of region and stage settings.  This otherwise locks in a multitude of bugs around the improper algorithm for selecting (given all context) the proper region or stage setting.  Instead, all code should use the centralized algorithm for determining such values.  This creates a strange first and second class configuration concept but these two are sufficiently varied and complex in their creation and use that this seems appropriate.
@erikerikson

This comment has been minimized.

Member

erikerikson commented Dec 12, 2017

While these changes seem to be the "right" thing for the framework, there is a risk implicit that these changes could negatively impact plugins if it creates an asynchronicity between plugin's expectations of the frameworks evaluation of these values from the context and its own evaluation of them from the context (which may well be copied code from the framework). However, for the long term centralizing such algorithms (if not some time generalizing them) seems the right anti-entropy choice.

@HyperBrain

A big step in the right direction!

@HyperBrain HyperBrain added this to the 1.25 milestone Dec 12, 2017

@@ -20,12 +21,13 @@ describe('AwsRollback', () => {
region: 'us-east-1',
timestamp: 1476779096930,
};
serverless.setProvider('aws', new AwsProvider(serverless));
provider = new AwsProvider(serverless);

This comment has been minimized.

@HyperBrain

HyperBrain Dec 12, 2017

Member

@erikerikson The AwsProvider constructor in the tests should include the options parameter, as AwsProvider.options is evaluated to fetch the stage in getStage(). Otherwise it will default to whatever is set as default or set in serverless.config.stage or serverless.service.provider.stage.
E.g. the test options here set it to dev (which is coincidentally the default), but when setting it here to someStage it will fail, because you did not give the actual options to the constructor.

This comment has been minimized.

@erikerikson

erikerikson Dec 12, 2017

Member

Good catch. Incoming in a moment.

This comment has been minimized.

@erikerikson

erikerikson Dec 12, 2017

Member

Looks like there were others (a search for new AwsProvider(serverless) uncovered them easily). Given that no tests are failing, I'll add them in the cases where an options hive is defined but will otherwise not provide the parameter. Let me know if you think I should be more aggressive on that front.

This comment has been minimized.

@HyperBrain

HyperBrain Dec 12, 2017

Member

Be as aggressive as you have to be :-D
Imo, if there are explicit test options defined, they should be given to the provider.

This comment has been minimized.

@erikerikson

erikerikson Dec 12, 2017

Member

This and others are addressed in the latest commit.

@horike37

Thanks @erikerikson 👍
LGTM since all tests have been passed.

However, for the long term centralizing such >algorithms (if not some time generalizing them) >seems the right anti-entropy choice.

You are right. The only thing for that quite recently is that doing test
thoroughly on the master branch before next release.

Remove unused parameters in platform.js (and deal with consequences)
Add explicit use of options where they were defined.
@erikerikson

This comment has been minimized.

Member

erikerikson commented Dec 12, 2017

This should be GTM unless Frank desires a more aggressive provision of option hives.

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 12, 2017

I'm perfectly fine with that :) The other occurences that are out of scope here can be touched when someone works there.

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 12, 2017

For the coverage decrease -> let's ignore that :) it seems it just uses different code branches now and draws in some files...

@HyperBrain HyperBrain merged commit f62116c into serverless:master Dec 12, 2017

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 88.085%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@erikerikson erikerikson deleted the erikerikson:fix-resolve-signature-and-calls branch Dec 12, 2017

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 12, 2017

BTW: Coveralls seems to be drunk:
image
The merge even increased the coverage...

@erikerikson

This comment has been minimized.

Member

erikerikson commented Dec 12, 2017

Haha. Thanks for calling this out, the laugh was pleasant!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment