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

Remove default stage value in provider object #6200

Merged
merged 3 commits into from Jun 21, 2019

Conversation

@mydiemho
Copy link
Contributor

commented May 31, 2019

What did you implement:

the current default is using AWS region naming syntax,
this mean other cloud provider would have to add extra code
to set their default value.

Instead, region default should be left to each provider.

How did you implement it:

one line code change

How can we verify it:

  1. Install this branch
  2. Run sls deploy in 3 different scenario:
    • set region as a flag
    • set region in serverless.yml
    • don't set region in flag or serverless.yml

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

@pmuens
Copy link
Member

left a comment

Hey @mydiemho thanks for working on this.

It looks like a test is failing. Can you it and rebase this PR. After that it should be in a good position for a review. Thanks!

@pmuens pmuens changed the title remove default stage value in provider object Remove default stage value in provider object Jun 5, 2019

mydiemho added 2 commits May 31, 2019
remove default stage value in provider object
the current default is using AWS region naming syntax,
this mean other cloud provider would have to add extra code
to set their default value.

Instead, region default should be left to each provider.

@mydiemho mydiemho force-pushed the mydiemho:myho/removeRegionDefaultFromBase branch from ee1ba97 to 5541f2b Jun 5, 2019

@mydiemho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

travis doesn't seem to be trigger, gonna close and reopen

@mydiemho mydiemho closed this Jun 5, 2019

@mydiemho mydiemho reopened this Jun 5, 2019

@mydiemho mydiemho marked this pull request as ready for review Jun 5, 2019

@mydiemho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@pmuens not sure if it's GitHub or Travis end, but travis build is not getting trigger

@pmuens pmuens added pr/in-review and removed pr/in-progress labels Jun 6, 2019

@pmuens
Copy link
Member

left a comment

Hey @mydiemho thanks for updating the PR 👍

Yes, sometimes Travis has some hiccups. Seems like the build was running and is now successful (I had to fix some minor linting issues and pushed the changes directly to your branch).

I tested this bu deploying one stack to the default region (us-east-1), by using the --region option and the provider.region config in serverless.yml. Everything was working fine.

Usually removing stuff from the core is a tricky thing because it can result in breaking changes. I faced the same issue you described while working on the Google Cloud Functions plugin. Looking at the code the change which is introduced here is bound to a provider, so usually it should be fine to remove the region here since other providers can't make use of it anyway (at least as far as I'm aware).

However, I'd love to get some more feedback on this. @medikoo could you also glance over this? Would this removal result in a breaking change 🤔

@pmuens pmuens self-assigned this Jun 6, 2019

@pmuens pmuens requested a review from medikoo Jun 6, 2019

@medikoo
Copy link
Member

left a comment

I'm totally in for this change (great thanks @mydiemho !)

I believe at core we should be provider agnostic, and this cleanup seems desirable.

Assuming us-east-1 is AWS default, and this change doesn't effectively change anything for AWS deployment, this seems non-breaking.

But maybe some plugins depend on existence of it? @pmuens if you feel it's risky we may mark it as potentially breaking and ship with nearest "major" (or breaking) release.

@mydiemho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@medikoo I can guarantee this won't break Azure haha, not sure about Google Cloud.

@mydiemho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Yes, sometimes Travis has some hiccups. Seems like the build was running and is now successful (I had to fix some minor linting issues and pushed the changes directly to your branch).

@pmuens thanks for the lint fix. What command did you use for the fix? We should add that as a npm script so others can use it.

@pmuens
pmuens approved these changes Jun 7, 2019
Copy link
Member

left a comment

@mydiemho I'm using an extension in my IDE. That way I see ESLint issue right away.

So I looked into this again today. Read through the provider codebases under the serverless org and couldn't find any evidence that this would be breaking. I agree that the core should be provider agnostic and we should remove this code here.

I'd propose that we discuss this with the team to get more feedback on this. Maybe @eahefnawy know why this code was added there. Once confirmed we can merge this right away.

@pmuens pmuens added this to In progress in Serverless via automation Jun 7, 2019

@pmuens pmuens added this to the 1.45.0 milestone Jun 7, 2019

@medikoo medikoo modified the milestones: 1.45.0, 1.46.0 Jun 11, 2019

@pmuens pmuens modified the milestones: 1.46.0, 1.47.0 Jun 19, 2019

@medikoo
Copy link
Member

left a comment

@eahefnawy pointed me that it won't work if we wouldn't specify region for AWS (there's no default region on AWS level)

Still in AWS provider logic individually we provide a fallback to 'us-east-1', and in light of that, this update seems a desirable cleanup

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

@pmuens
pmuens approved these changes Jun 21, 2019
Copy link
Member

left a comment

Thanks for the update @medikoo 👍

Yes, that sounds reasonable. I tested it again today and surely enough the default region kicks-in for AWS deployments. Given that this is the case I'd say it's GTM :shipit:

@pmuens pmuens modified the milestones: 1.47.0, 1.46.0 Jun 21, 2019

@pmuens pmuens merged commit 6d05d83 into serverless:master Jun 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@mydiemho mydiemho deleted the mydiemho:myho/removeRegionDefaultFromBase branch Jun 21, 2019

@aaronjensen

This comment has been minimized.

Copy link

commented Jun 26, 2019

This is a breaking change for serverless offline and serverless dynamodb local.

@medikoo

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

This is a breaking change for serverless offline and serverless dynamodb local.

@aaronjensen can you share more details? I just tested serverless-offline with v1.46.0 and it worked without issues for me.

@moshebs

This comment has been minimized.

Copy link

commented Jun 27, 2019

Not sure if I am doing something wrong, but seems like this change is a breaking one, for me.

When I do not configure a region anywhere for the Serverless Framework (not as cli param and not in the serverless.yml), and then I relate to self:provider.region, I get no value as of v1.46.0. This worked with 1.45.1.

@aaronjensen

This comment has been minimized.

Copy link

commented Jun 27, 2019

@medikoo I'm not sure, I've fixed it by manually setting region in serverless.yml. This was the error I got in CI when creating/migrating the dynamodb local tables:

 $ DEPLOY_ENV=test serverless dynamodb migrate --stage test || true
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid option to satisfy the declaration 'opt:region,self:provider.region' could not be found.
 
 
  Serverless Error ---------------------------------------
 
  Trying to populate non string value into a string for variable ${opt:region, self:provider.region}. Please make sure the value of the property is a string
@medikoo

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Ok, it's that references toself:provider.region in serverless.yaml what got broken.

It's true, it's no longer there (if you do not set region in serverless.yaml directly). It indeed is a regression, we'll fix it shortly.

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.