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

update calls to aws.request to use new API (fixes #41) #42

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

linusmarco
Copy link
Contributor

Background

As described in #41, our calls to the AWS SDK request method use the old API, which has arguments for region and stage. The new API does not support these arguments, instead storing the region and stage elsewhere. Instead there is an optional options argument (which we don't need for anything in serverless-finch at the moment). Region and stage are set at initialization.

Proposed changes

Change all calls to aws.request to remove the region and stage arguments. Example:

return this.aws.request('S3', 'putObject', params, this.stage, this.region);
return this.aws.request('S3', 'putObject', params);

@fernando-mc
Copy link
Owner

@linusmarco I'd be super surprised if this doesn't break anything. Have you already tested it?

@linusmarco
Copy link
Contributor Author

@fernando-mc, yup I tested it and everything works. The way the updates Serverless API works, the region and stage are taken from serverless.yml on initialization of the Serverless class. You can check out the signature of the request function here:
https://github.com/serverless/serverless/blob/master/lib/plugins/aws/provider/awsProvider.js#L196

Right now, we're passing stage in the options argument, which ends up getting ignored/overridden on line 200 in the file linked above.

Someone else should also test to make sure, though

@fernando-mc fernando-mc merged commit 2ec6c0f into fernando-mc:master Mar 12, 2018
@fernando-mc
Copy link
Owner

@linusmarco tested. This works.

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