Remove not used parameters from request() and add options hive

[HyperBrain]

What did you implement:

There were many places were calls to provider.request() received a stage and region parameter, although these are not used since a long time anymore even if the function signature was changed too.

How did you implement it:

Removed the not-used parameters in all calls.

Additionally the new useCache parameter was too specific to be part of the signature, so I added an options object, that can be optionally given and is used as container for options that change the request method's behavior.

How can we verify it:

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

[HyperBrain]

Seems that I catched all of them.

[tommedema]

@HyperBrain the options argument is great. Will this indeed allow me to use provider.request('S3', 'upload', ...) with a second parameter "options"?

E.g. to enable multipart uploading as per the AWS docs (this is currently not possible as provider.request only accepts 1 params object and not an options object):

provider.request('S3', 'upload', params, {
  partSize: 5 * 1024 * 1024,
  queueSize: 10
})

[HyperBrain]

@tommedema , the options hive here is meant to configure the request execution behavior. E.g. you can set options: useCache: true to let request be cached (if the caller's use case supports that).

You specific use case for the upload is out of scope for this PR, but could be added as a second step ,either by having some options.params2 or allow params to be an array of params (in addition to the current state) for the request function.
However, I favor the array semantics, as it expresses that there can be multiple parameters passed to the request function. Options is something different and defines the local behavior rather than the behavior changes configured at the AWS SDK.

[HyperBrain]

@horike37 This must get in now that the vars branch has been merged. It removes the wrong/stale request method signature and corrects the unit tests. Can you please review and approve, so that I can get it in?

[horike37]

Thank you for refactoring around request function @HyperBrain 👍

I tried sls deploy on this PR branch, and when enabling SLS_DEBUG, saw WARNING: Inappropriate call of provider.request error continuously as follow.

Was not this done intentionally?

$../serverless/bin/serverless deploy
Serverless: Creating Stack...
Serverless: WARNING: Inappropriate call of provider.request()
Serverless: Checking Stack create progress...
Serverless: WARNING: Inappropriate call of provider.request()
..
Serverless: WARNING: Inappropriate call of provider.request()
.
Serverless: WARNING: Inappropriate call of provider.request()
undefined
Serverless: WARNING: Inappropriate call of provider.request()
Serverless: WARNING: Inappropriate call of provider.request()
Serverless: WARNING: Inappropriate call of provider.request()
..
Serverless: Stack create finished...
Serverless: WARNING: Inappropriate call of provider.request()
Serverless: WARNING: Inappropriate call of provider.request()
Serverless: Uploading CloudFormation file to S3...
Serverless: WARNING: Inappropriate call of provider.request()
Serverless: Uploading artifacts...

[HyperBrain]

@horike37 Do you have any plugin enabled?

It will log a warning if someone (a plugin) calls request with the wrong signature (including stage and region) to find misuses. However without any plugins enabled it should not log the message.

[horike37]

Sorry, I saw old commits. I did pull again and it works correctly 💦
I tested it with some commands which are probably affected by this fix like deploy, function deploy, remove and so on.

Then all commands work perfectly. Seems everything is good 💯
merging...

[erikerikson]

Sorry I didn't get a review in before this was merged but it looked good. I did have the comment that there was pending dependencies on the use of options.stage && options.region throughout the codebase. (e.g. https://github.com/serverless/serverless/blob/master/lib/plugins/aws/remove/lib/bucket.js#L7 )(another example: https://github.com/serverless/serverless/blob/master/lib/plugins/platform/platform.js#L37 - I don't understand this one and it seems AWS specific despite being not within the scope of the AWS plugin... Weird to me). However, I wanted to provide code of what I meant rather than just call it out and I delayed feedback as a result.
I have created a test and lint passing implementation of this at:
#4560

[HyperBrain]

@erikerikson You're right. For the getDeploymentBucket() in aws/remove, the stage/region retrieval should be included within the function itself utilizing the central provider.getStage() and provider.getRegion() functions. For the platform plugin, it seems that I missed the wrong signature in there - however I do not know if that plugin is bound to AWS. However, the parameters in request were not used anymore for a long time - so there they had no effect at all ...

[erikerikson]

Thanks for confirming. I'll add that on #4560. Hadn't been sure on scope.