From c2c86a0c32963b3e2c55d554c243907d63b564b1 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Wed, 7 Aug 2019 18:09:43 -0700 Subject: [PATCH 1/5] fix: Removed service name from storage account name generator --- .../resources/storageAccount.test.ts | 5 +---- src/services/namingService.ts | 17 ++++++++--------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/armTemplates/resources/storageAccount.test.ts b/src/armTemplates/resources/storageAccount.test.ts index b398cda6..4d158eab 100644 --- a/src/armTemplates/resources/storageAccount.test.ts +++ b/src/armTemplates/resources/storageAccount.test.ts @@ -1,4 +1,3 @@ -import md5 from "md5"; import { ServerlessAzureConfig } from "../../models/serverless"; import { AzureNamingService } from "../../services/namingService"; import { StorageAccountResource } from "./storageAccount"; @@ -35,8 +34,7 @@ describe("Storage Account Resource", () => { prefix: "my-long-test-prefix-name", region: "Australia Southeast", stage: "development" - }, - service: "my-long-test-api", + } }; const result = StorageAccountResource.getResourceName(testConfig); @@ -145,7 +143,6 @@ describe("Storage Account Resource", () => { expect(value).toContain(AzureNamingService.createShortAzureRegionName(config.provider.region)); expect(value).toContain(createSafeString(config.provider.prefix)); expect(value).toContain(createSafeString(config.provider.stage)); - expect(value).toContain(md5(config.service).substr(0, 3)); } function createSafeString(value: string) { diff --git a/src/services/namingService.ts b/src/services/namingService.ts index 10c37550..6d886905 100644 --- a/src/services/namingService.ts +++ b/src/services/namingService.ts @@ -53,24 +53,23 @@ export class AzureNamingService { const { prefix, region, stage } = config.provider; - const nameHash = md5(config.service); - let safePrefix = prefix.replace(forbidden, replaceWith); const safeRegion = this.createShortAzureRegionName(region); let safeStage = this.createShortStageName(stage); - let safeNameHash = nameHash.substr(0, 6); - const remaining = maxLength - (safePrefix.length + safeRegion.length + safeStage.length + safeNameHash.length); + const remaining = maxLength - (safePrefix.length + safeRegion.length + safeStage.length); // Dynamically adjust the substring based on space needed if (remaining < 0) { - const partLength = Math.floor(Math.abs(remaining) / 3); + let partLength = Math.floor(Math.abs(remaining) / 2); + if (partLength < 3) { + partLength = 3; + } safePrefix = safePrefix.substr(0, partLength); safeStage = safeStage.substr(0, partLength); - safeNameHash = safeNameHash.substr(0, partLength); } - return [safePrefix, safeRegion, safeStage, safeNameHash] + return [safePrefix, safeRegion, safeStage] .join("") .toLocaleLowerCase(); } @@ -85,8 +84,8 @@ export class AzureNamingService { maxLength -= timestamp.length + suffix.length;; const name = (deploymentName) ? deploymentName.substr(0, maxLength) - : [ AzureNamingService.getSafeResourceName(config, maxLength), suffix ].join("-"); - return [ name, timestamp ].join("-"); + : [AzureNamingService.getSafeResourceName(config, maxLength), suffix].join("-"); + return [name, timestamp].join("-"); } return deploymentName.substr(0, maxLength); From 0b9b737abd1dd447fdab9d0d1690d6f955533ecc Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 8 Aug 2019 12:14:49 -0700 Subject: [PATCH 2/5] test: Ensure deployment name contains service name part --- package-lock.json | 20 ------- package.json | 1 - src/armTemplates/resources/apim.ts | 6 +- src/armTemplates/resources/appInsights.ts | 6 +- src/armTemplates/resources/appServicePlan.ts | 6 +- src/armTemplates/resources/functionApp.ts | 7 +-- .../resources/hostingEnvironment.ts | 6 +- .../resources/storageAccount.test.ts | 2 +- src/armTemplates/resources/storageAccount.ts | 6 +- src/services/namingService.test.ts | 58 ++++++++++++++++++- src/services/namingService.ts | 29 ++++++---- 11 files changed, 84 insertions(+), 63 deletions(-) diff --git a/package-lock.json b/package-lock.json index e64a9f6d..fecdc6a5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2852,11 +2852,6 @@ "integrity": "sha512-mT8iDcrh03qDGRRmoA2hmBJnxpllMR+0/0qlzjqZES6NdiWDcZkCNAk4rPFZ9Q85r27unkiNNg8ZOiwZXBHwcA==", "dev": true }, - "charenc": { - "version": "0.0.2", - "resolved": "https://registry.npmjs.org/charenc/-/charenc-0.0.2.tgz", - "integrity": "sha1-wKHS86cJLgN3S/qD8UwPxXkKhmc=" - }, "chokidar": { "version": "2.1.6", "resolved": "https://registry.npmjs.org/chokidar/-/chokidar-2.1.6.tgz", @@ -3306,11 +3301,6 @@ "which": "^1.2.9" } }, - "crypt": { - "version": "0.0.2", - "resolved": "https://registry.npmjs.org/crypt/-/crypt-0.0.2.tgz", - "integrity": "sha1-iNf/fsDfuG9xPch7u0LQRNPmxBs=" - }, "crypto-browserify": { "version": "3.12.0", "resolved": "https://registry.npmjs.org/crypto-browserify/-/crypto-browserify-3.12.0.tgz", @@ -7573,16 +7563,6 @@ "object-visit": "^1.0.0" } }, - "md5": { - "version": "2.2.1", - "resolved": "https://registry.npmjs.org/md5/-/md5-2.2.1.tgz", - "integrity": "sha1-U6s41f48iJG6RlMp6iP6wFQBJvk=", - "requires": { - "charenc": "~0.0.1", - "crypt": "~0.0.1", - "is-buffer": "~1.1.1" - } - }, "md5.js": { "version": "1.3.5", "resolved": "https://registry.npmjs.org/md5.js/-/md5.js-1.3.5.tgz", diff --git a/package.json b/package.json index 6018b19a..8305eba4 100644 --- a/package.json +++ b/package.json @@ -50,7 +50,6 @@ "js-yaml": "^3.13.1", "jsonpath": "^1.0.1", "lodash": "^4.16.6", - "md5": "^2.2.1", "open": "^6.3.0", "request": "^2.81.0", "rimraf": "^2.6.3", diff --git a/src/armTemplates/resources/apim.ts b/src/armTemplates/resources/apim.ts index 5003361c..2bd66521 100644 --- a/src/armTemplates/resources/apim.ts +++ b/src/armTemplates/resources/apim.ts @@ -5,11 +5,7 @@ import { AzureNamingService } from "../../services/namingService"; export class ApimResource implements ArmResourceTemplateGenerator { public static getResourceName(config: ServerlessAzureConfig) { - return AzureNamingService.getResourceName( - config, - config.provider.apim, - "apim" - ); + return AzureNamingService.getResourceName(config, config.provider.apim, "apim"); } public getTemplate(): ArmResourceTemplate { diff --git a/src/armTemplates/resources/appInsights.ts b/src/armTemplates/resources/appInsights.ts index 9426a0b6..ab72cc94 100644 --- a/src/armTemplates/resources/appInsights.ts +++ b/src/armTemplates/resources/appInsights.ts @@ -4,11 +4,7 @@ import { AzureNamingService } from "../../services/namingService"; export class AppInsightsResource implements ArmResourceTemplateGenerator { public static getResourceName(config: ServerlessAzureConfig) { - return AzureNamingService.getResourceName( - config, - config.provider.appInsights, - "appinsights" - ); + return AzureNamingService.getResourceName(config, config.provider.appInsights, "appinsights"); } public getTemplate(): ArmResourceTemplate { diff --git a/src/armTemplates/resources/appServicePlan.ts b/src/armTemplates/resources/appServicePlan.ts index 4485cb06..50d99e6d 100644 --- a/src/armTemplates/resources/appServicePlan.ts +++ b/src/armTemplates/resources/appServicePlan.ts @@ -4,11 +4,7 @@ import { AzureNamingService } from "../../services/namingService"; export class AppServicePlanResource implements ArmResourceTemplateGenerator { public static getResourceName(config: ServerlessAzureConfig) { - return AzureNamingService.getResourceName( - config, - config.provider.appInsights, - "asp" - ); + return AzureNamingService.getResourceName(config, config.provider.appInsights, "asp"); } public getTemplate(): ArmResourceTemplate { diff --git a/src/armTemplates/resources/functionApp.ts b/src/armTemplates/resources/functionApp.ts index 1af37a3e..0263490f 100644 --- a/src/armTemplates/resources/functionApp.ts +++ b/src/armTemplates/resources/functionApp.ts @@ -5,11 +5,8 @@ import { AzureNamingService } from "../../services/namingService"; export class FunctionAppResource implements ArmResourceTemplateGenerator { public static getResourceName(config: ServerlessAzureConfig) { const safeServiceName = config.service.replace(/\s/g, "-"); - return AzureNamingService.getResourceName( - config, - config.provider.appInsights, - safeServiceName - ); + + return AzureNamingService.getResourceName(config, config.provider.appInsights, safeServiceName); } public getTemplate(): ArmResourceTemplate { diff --git a/src/armTemplates/resources/hostingEnvironment.ts b/src/armTemplates/resources/hostingEnvironment.ts index 7c8332bd..0cf86716 100644 --- a/src/armTemplates/resources/hostingEnvironment.ts +++ b/src/armTemplates/resources/hostingEnvironment.ts @@ -4,11 +4,7 @@ import { AzureNamingService } from "../../services/namingService"; export class HostingEnvironmentResource implements ArmResourceTemplateGenerator { public static getResourceName(config: ServerlessAzureConfig) { - return AzureNamingService.getResourceName( - config, - config.provider.hostingEnvironment, - "ase" - ); + return AzureNamingService.getResourceName(config, config.provider.hostingEnvironment, "ase"); } public getTemplate(): ArmResourceTemplate { diff --git a/src/armTemplates/resources/storageAccount.test.ts b/src/armTemplates/resources/storageAccount.test.ts index 4d158eab..5d20643b 100644 --- a/src/armTemplates/resources/storageAccount.test.ts +++ b/src/armTemplates/resources/storageAccount.test.ts @@ -146,6 +146,6 @@ describe("Storage Account Resource", () => { } function createSafeString(value: string) { - return value.replace(/\W+/g, "").toLocaleLowerCase().substr(0, 3); + return value.replace(/\W+/g, "").toLowerCase().substr(0, 3); }; }); diff --git a/src/armTemplates/resources/storageAccount.ts b/src/armTemplates/resources/storageAccount.ts index 69dc7cd6..86832ae9 100644 --- a/src/armTemplates/resources/storageAccount.ts +++ b/src/armTemplates/resources/storageAccount.ts @@ -5,11 +5,7 @@ import configConstants from "../../config"; export class StorageAccountResource implements ArmResourceTemplateGenerator { public static getResourceName(config: ServerlessAzureConfig) { - return AzureNamingService.getSafeResourceName( - config, - configConstants.naming.maxLength.storageAccount, - config.provider.storageAccount - ); + return AzureNamingService.getSafeResourceName(config, configConstants.naming.maxLength.storageAccount, config.provider.storageAccount); } public getTemplate(): ArmResourceTemplate { diff --git a/src/services/namingService.test.ts b/src/services/namingService.test.ts index 33749c45..55afa935 100644 --- a/src/services/namingService.test.ts +++ b/src/services/namingService.test.ts @@ -1,7 +1,7 @@ import { AzureNamingService } from "./namingService" +import { ServerlessAzureConfig } from "../models/serverless"; describe("Naming Service", () => { - it("Creates a short name for an azure region", () => { const expected = "ausse"; const actual = AzureNamingService.createShortAzureRegionName("australiasoutheast"); @@ -86,4 +86,60 @@ describe("Naming Service", () => { const actual = AzureNamingService.getNormalizedRegionName(expected); expect(actual).toEqual(expected); }); + + it("deployment name is generated correctly", () => { + const config: ServerlessAzureConfig = { + functions: [], + plugins: [], + provider: { + prefix: "sls", + name: "azure", + region: "westus", + stage: "dev", + }, + service: "test-api" + }; + + const timestamp = Date.now().toString(); + + const deploymentName = AzureNamingService.getDeploymentName(config, `t${timestamp}`); + expect(deploymentName.length).toBeLessThanOrEqual(64); + expect(deploymentName).toContain(timestamp); + expect(deploymentName).toContain(AzureNamingService.createShortAzureRegionName(config.provider.region)); + expect(deploymentName).toContain(createSafeString(config.provider.prefix)); + expect(deploymentName).toContain(createSafeString(config.provider.stage)); + expect(deploymentName).toContain(createSafeString(config.service)); + }); + + it("deployment name with long suffix or service name generated correctly", () => { + const config: ServerlessAzureConfig = { + functions: [], + plugins: [], + provider: { + prefix: "sls-long-prefix-name", + name: "azure", + region: "westus", + stage: "multicloud", + }, + service: "extra-long-service-name" + }; + + const timestamp = Date.now(); + + const deploymentName = AzureNamingService.getDeploymentName(config, `t${timestamp}`); + assertValidDeploymentName(config, deploymentName, timestamp); + }); + + function assertValidDeploymentName(config: ServerlessAzureConfig, value: string, timestamp: number) { + expect(value.length).toBeLessThanOrEqual(64); + expect(value).toContain(timestamp); + expect(value).toContain(AzureNamingService.createShortAzureRegionName(config.provider.region)); + expect(value).toContain(createSafeString(config.provider.prefix)); + expect(value).toContain(createSafeString(config.provider.stage)); + expect(value).toContain(createSafeString(config.service)); + } + + function createSafeString(value: string) { + return value.replace(/\W+/g, "").toLowerCase().substr(0, 3); + }; }); diff --git a/src/services/namingService.ts b/src/services/namingService.ts index 6d886905..29559a8e 100644 --- a/src/services/namingService.ts +++ b/src/services/namingService.ts @@ -1,6 +1,5 @@ import { ServerlessAzureConfig, ResourceConfig } from "../models/serverless" import { Guard } from "../shared/guard" -import md5 from "md5"; import configConstants from "../config"; export class AzureNamingService { @@ -18,16 +17,19 @@ export class AzureNamingService { if (resourceConfig && resourceConfig.name) { return resourceConfig.name; } + const { prefix, region, stage } = config.provider let name = [ prefix, this.createShortAzureRegionName(region), this.createShortStageName(stage), ].join("-"); + if (suffix) { name += `-${suffix}`; } - return name.toLocaleLowerCase(); + + return name.toLowerCase(); } /** @@ -42,12 +44,14 @@ export class AzureNamingService { * @param forbidden Regex for characters to remove from name. Defaults to non-alpha-numerics * @param replaceWith String to replace forbidden characters. Defaults to empty string */ - public static getSafeResourceName(config: ServerlessAzureConfig, maxLength: number, resourceConfig?: ResourceConfig, forbidden = /\W+/g, replaceWith = "") { + public static getSafeResourceName(config: ServerlessAzureConfig, maxLength: number, resourceConfig?: ResourceConfig, suffix: string = "", forbidden: RegExp = /\W+/g, replaceWith: string = "") { if (resourceConfig && resourceConfig.name) { const { name } = resourceConfig; + if (name.length > maxLength) { throw new Error(`Name '${name}' invalid. Should be shorter than ${maxLength} characters`); } + return name.replace(forbidden, replaceWith); } @@ -56,22 +60,25 @@ export class AzureNamingService { let safePrefix = prefix.replace(forbidden, replaceWith); const safeRegion = this.createShortAzureRegionName(region); let safeStage = this.createShortStageName(stage); + let safeSuffix = suffix.replace(forbidden, replaceWith); - const remaining = maxLength - (safePrefix.length + safeRegion.length + safeStage.length); + const remaining = maxLength - (safePrefix.length + safeRegion.length + safeStage.length + safeSuffix.length); // Dynamically adjust the substring based on space needed if (remaining < 0) { - let partLength = Math.floor(Math.abs(remaining) / 2); + let partLength = Math.floor(Math.abs(remaining) / 4); if (partLength < 3) { partLength = 3; } + safePrefix = safePrefix.substr(0, partLength); safeStage = safeStage.substr(0, partLength); + safeSuffix = safeSuffix.substr(0, partLength); } - return [safePrefix, safeRegion, safeStage] + return [safePrefix, safeRegion, safeStage, safeSuffix] .join("") - .toLocaleLowerCase(); + .toLowerCase(); } public static getDeploymentName(config: ServerlessAzureConfig, timestamp?: string) { @@ -81,10 +88,12 @@ export class AzureNamingService { const { deploymentName } = config.provider if (timestamp) { - maxLength -= timestamp.length + suffix.length;; + maxLength -= timestamp.length + suffix.length; + + const name = (deploymentName) + ? deploymentName.substr(0, maxLength) + : [AzureNamingService.getSafeResourceName(config, maxLength, null, config.service), suffix].join("-"); - const name = (deploymentName) ? deploymentName.substr(0, maxLength) - : [AzureNamingService.getSafeResourceName(config, maxLength), suffix].join("-"); return [name, timestamp].join("-"); } From bc01b1c2cc193d04db1703864398a788e327d254 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 8 Aug 2019 12:47:48 -0700 Subject: [PATCH 3/5] doc: Updated js docs based on PR feedback --- src/services/namingService.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/services/namingService.ts b/src/services/namingService.ts index 29559a8e..8d7df131 100644 --- a/src/services/namingService.ts +++ b/src/services/namingService.ts @@ -10,8 +10,8 @@ export class AzureNamingService { * {prefix}-{shortRegionName}-{shortStageName}(optionally: -{suffix}) * * @param config Serverless Azure Config for service (serverless.service) - * @param resourceConfig - * @param suffix + * @param resourceConfig The serverless resource configuration + * @param suffix Optional suffix to append on the end of the generated name */ public static getResourceName(config: ServerlessAzureConfig, resourceConfig?: ResourceConfig, suffix?: string) { if (resourceConfig && resourceConfig.name) { @@ -40,7 +40,8 @@ export class AzureNamingService { * * @param config Serverless Azure Config for service (serverless.service) * @param maxLength Maximum length of name for resource - * @param resourceConfig Configuration for resource from serverless configuration + * @param resourceConfig The serverless resource configuration + * @param suffix Optional suffix to append on the end of the generated name * @param forbidden Regex for characters to remove from name. Defaults to non-alpha-numerics * @param replaceWith String to replace forbidden characters. Defaults to empty string */ @@ -81,6 +82,11 @@ export class AzureNamingService { .toLowerCase(); } + /** + * Creates a deployment name from the serverless configuration + * @param config The serverless azure config + * @param timestamp The timestamp of the deployment + */ public static getDeploymentName(config: ServerlessAzureConfig, timestamp?: string) { let maxLength = configConstants.naming.maxLength.deploymentName; const suffix = configConstants.naming.suffix.deployment; From 04ea605213734da9ff5d243818b7b51f9a1c3b81 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 8 Aug 2019 12:56:58 -0700 Subject: [PATCH 4/5] fix: Updated test case to reuse 'assertDeploymentName` --- src/services/namingService.test.ts | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/services/namingService.test.ts b/src/services/namingService.test.ts index 55afa935..9ee663a2 100644 --- a/src/services/namingService.test.ts +++ b/src/services/namingService.test.ts @@ -100,15 +100,10 @@ describe("Naming Service", () => { service: "test-api" }; - const timestamp = Date.now().toString(); - + const timestamp = Date.now(); const deploymentName = AzureNamingService.getDeploymentName(config, `t${timestamp}`); - expect(deploymentName.length).toBeLessThanOrEqual(64); - expect(deploymentName).toContain(timestamp); - expect(deploymentName).toContain(AzureNamingService.createShortAzureRegionName(config.provider.region)); - expect(deploymentName).toContain(createSafeString(config.provider.prefix)); - expect(deploymentName).toContain(createSafeString(config.provider.stage)); - expect(deploymentName).toContain(createSafeString(config.service)); + + assertValidDeploymentName(config, deploymentName, timestamp); }); it("deployment name with long suffix or service name generated correctly", () => { @@ -125,8 +120,8 @@ describe("Naming Service", () => { }; const timestamp = Date.now(); - const deploymentName = AzureNamingService.getDeploymentName(config, `t${timestamp}`); + assertValidDeploymentName(config, deploymentName, timestamp); }); From 82d96dd3f8d8199400612a0fee1f8df93742a3a1 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 8 Aug 2019 13:00:37 -0700 Subject: [PATCH 5/5] test: Added more explicit comparison on deployment name --- src/services/namingService.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/services/namingService.test.ts b/src/services/namingService.test.ts index 9ee663a2..d58fbc16 100644 --- a/src/services/namingService.test.ts +++ b/src/services/namingService.test.ts @@ -103,6 +103,7 @@ describe("Naming Service", () => { const timestamp = Date.now(); const deploymentName = AzureNamingService.getDeploymentName(config, `t${timestamp}`); + expect(deploymentName).toEqual(`slswusdevtestapi-DEPLOYMENT-t${timestamp}`); assertValidDeploymentName(config, deploymentName, timestamp); }); @@ -122,6 +123,7 @@ describe("Naming Service", () => { const timestamp = Date.now(); const deploymentName = AzureNamingService.getDeploymentName(config, `t${timestamp}`); + expect(deploymentName).toEqual(`slswusmulext-DEPLOYMENT-t${timestamp}`); assertValidDeploymentName(config, deploymentName, timestamp); });