Azure Serverless plugin #2
Conversation
|
@worldsoup @ac360 - This is the initial PR which has the core pieces. Once we get this in, will send updates with improvements. |
|
Adding @yochay as FYI |
|
@pragnagopa what happened with the semicolons? |
|
Fixed semicolons. Auto fixing via Eslint removed them |
There was a problem hiding this comment.
Great work @pragnagopa !
The package.json seems like a generated one ? You could probably clean this.
package.json
Outdated
| "main": "index.js", | ||
| "name": "serverless-azure-functions", | ||
| "optionalDependencies": {}, | ||
| "readme": "# serverless-azure-functions\r\nWIP – Coming soon...\r\n", |
There was a problem hiding this comment.
Do we need this if we have the readmeFilename?
shared/parseBindings.js
Outdated
| module.exports = { | ||
| getBindingsMetaData: function (serverless) { | ||
| serverless.cli.log(`Parsing Azure Functions Bindings.json...`); | ||
| var bindingDisplayNames = []; |
There was a problem hiding this comment.
Most of these vars could be const
shared/utils.js
Outdated
| bindingSettingsNames = parsedBindings['bindingSettingsNames'][bindingTypeIndex]; | ||
|
|
||
| if (azureSettings) { | ||
| for (var j = 0; j < Object.keys(azureSettings).length; j++) { |
There was a problem hiding this comment.
Loops should use let instead of var unless you mean to use the variable outside the block scope
package.json
Outdated
| "bluebird": "^3.4.6", | ||
| "chalk": "^1.1.3", | ||
| "lodash": "^4.16.6", | ||
| "ms-rest-azure": "*", |
There was a problem hiding this comment.
not safe to use * versions
There was a problem hiding this comment.
I think the request module is missing from this list, too.
|
You ought to be able to set your ESLint settings to warn you about using Also, since we're targeting es6, we might want to add an "engine" version to prevent folks from using older Node versions - ideally folks are using 6.5.0 given that's what we're using in Azure right now. |
|
If you need help fixing the code review remarks above or anything else, please let me know. I would love to help with anything to get proper Azure support in Serverless. |
provider/azureProvider.js
Outdated
| this.serverless = serverless; | ||
| this.provider = this; | ||
| this.serverless.setProvider(constants.providerName, this); | ||
| subscriptionId = process.env[this.serverless.service.provider.subscriptionId]; |
There was a problem hiding this comment.
This doesn't seem correct. It seems like this used to pull credentials from serverless.yml and then switched to taking them from environment variables, but now instead it kind of does half of both. Environment variables seem better than serverless.yml, because at least it's out of the source, but it's still a worse pattern than the other plugins have. I'm not sure what options there are with Azure credentials, though. Anyways, the way the README currently reads, it seems like
subscriptionId = process.env['azureSubId'];
servicePrincipalTenantId = process.env['azureServicePrincipalTenantId'];
servicePrincipalClientId = process.env['azureservicePrincipalClientId']; // The 's' should probably get capitalized in the README
servicePrincipalPassword = process.env['azureServicePrincipalPassword'];would be more correct.
|
@mgarrettm - For now using the environment variables for azure credentials, but we are looking into other options to login - command line login , providing a script to setup credentials. |
|
@pragnagopa - Both of those directions sounds promising, thanks for the update. And thanks for the plugin, Serverless support for Azure is really awesome to see! |
No description provided.