fix: Sort deployments in descending order and fix APIM arm template #354

tbarlow12 · 2019-09-30T20:34:39Z

What did you implement:

Updated parameter name in APIM arm template
Fixed bug of sorting deployments in ascending order, when it should have been descending. This would have pretty serious consequences, because it means that the comparison of ARM templates would always be targeting the first ever deployment, not the most recent.
Because the sort() function sorts the array in place, this bug was not being detected by the tests. Updated resourceService tests to create copies of the array rather than using the original reference when testing the validity of the result.

How did you implement it:

Flipped the timestamp comparison operator
Updated the string in apim.ts

How can we verify it:

Deploy a service
Make changes to an environment variable
Deploy it again (should deploy ARM template)
Change back to the original configuration
Deploy it again (should deploy ARM template)

Without this fix, the last step would not have deployed the ARM template since it would be identical to the first deployment ever.

Todos:

Note: Run npm run test:ci to run all validation checks on proposed changes

Write tests and confirm existing functionality is not broken.
Validate via npm test
Write documentation
Ensure there are no lint errors.
Validate via npm run lint
Note: Some reported issues can be automatically fixed by running npm run lint:fix
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

src/services/resourceService.ts

mydiemho

looks good, I have a few questions though

src/services/resourceService.test.ts

wbreza

Added a few minor comments - otherwise looks good 🚢

wbreza · 2019-10-02T23:51:58Z

src/armTemplates/resources/apim.ts

@@ -61,7 +61,7 @@ export class ApimResource implements ArmResourceTemplateGenerator {
          "location": "[parameters('location')]",
          "sku": {
            "name": "[parameters('apimSkuName')]",
-            "capacity": "[parameters('apimCapacity')]"
+            "capacity": "[parameters('apimSkuCapacity')]"


Nice catch!

src/services/resourceService.test.ts

wbreza · 2019-10-02T23:53:09Z

src/services/resourceService.test.ts

@@ -102,10 +105,10 @@ describe("Resource Service", () => {
    const service = new ResourceService(sls, options);
    const deploymentString = await service.listDeployments();
    let expectedDeploymentString = "\n\nDeployments";
-    const originalTimestamp = +MockFactory.createTestTimestamp();
+    const originalTimestamp = +MockFactory.createTestTimestamp() + 4;


Nit: Missing semicolon at end of line.

wbreza · 2019-10-02T23:53:28Z

src/services/resourceService.test.ts

-    for (const dep of deployments) {
-      const timestamp = originalTimestamp + i
+    for (const dep of [...deployments].reverse()) {
+      const timestamp = originalTimestamp - i


Nit: Missing semicolon at end of line.

…354) - Updated parameter name in APIM arm template - Fixed bug of sorting deployments in ascending order, when it should have been descending. This would have pretty serious consequences, because it means that the comparison of ARM templates would always be targeting the first ever deployment, not the most recent. - Because the `sort()` function sorts the array in place, this bug was not being detected by the tests. Updated `resourceService` tests to create copies of the array rather than using the original reference when testing the validity of the result.

* feat: ConfigService for centralizing configuration and simplifying BaseService (#338) `ConfigService` and its usage. This removes the configuration logic from `BaseService` and keeps it all in one place. Also adds some constants for default configuration. * release: Update prerelease version to 1.0.2-0 ***NO_CI*** * fix: Sync triggers on external package deployment (#339) Fix updating of function app settings (SDK call wasn't working) and syncing triggers for function apps running from external package. * release: Update prerelease version to 1.0.2-1 ***NO_CI*** * ci: fix failing Node 8 builds on windows agent (#345) Hosted agent roll out a fix that broke our builds: 1. Previously, npm wasn’t getting packaged with the version of node in the tool cache, ie. npm 5.6.0 should be used alongside Node 8.10.0. 1. The fix is to pin to a later version of Node 8 (e.g. 8.16.1) which comes with npm 6+ - https://nodejs.org/en/download/releases/. * This will probably slow the build down a little bit since the agent will have to download the version (instead of it being pre-installed), but we'll get the right version of npm for free. * fix job name * fix job name restrictiosn * still trying to get the right job name format * clean up job name * release: Update prerelease version to 1.0.2-2 ***NO_CI*** * release: Update prerelease version to 1.0.2-3 ***NO_CI*** * fix: Fix typing errors in ARM params and add interfaces (#347) * release: Update prerelease version to 1.0.2-4 ***NO_CI*** * fix: Update to support CosmosDB bindings (#350) Updatings the binding schema that is generated to support the changes made to Cosmos DB * release: Update prerelease version to 1.0.2-5 ***NO_CI*** * feat: Refactor runtime configuration to allow for non-node runtimes (#348) - Added `FunctionRuntime` configuration to provider - Extracting `FunctionRuntime` from `runtime` property of configuration within `ConfigService` - Refactored node-specific code in ARM template generation * release: Update prerelease version to 1.0.2-6 ***NO_CI*** * fix: Update GitHub Issue and PR templates (#353) * release: Update prerelease version to 1.0.2-7 ***NO_CI*** * fix: Sort deployments in descending order and fix APIM arm template (#354) - Updated parameter name in APIM arm template - Fixed bug of sorting deployments in ascending order, when it should have been descending. This would have pretty serious consequences, because it means that the comparison of ARM templates would always be targeting the first ever deployment, not the most recent. - Because the `sort()` function sorts the array in place, this bug was not being detected by the tests. Updated `resourceService` tests to create copies of the array rather than using the original reference when testing the validity of the result. * release: Update prerelease version to 1.0.2-8 ***NO_CI*** * release: Update patch version to 1.0.2 ***NO_CI*** * ci: Add GitHub workflow to move new issues to "To triage" column (#381) * build(deps): bump handlebars from 4.1.2 to 4.5.3 (#400) Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.1.2 to 4.5.3. - [Release notes](https://github.com/wycats/handlebars.js/releases) - [Changelog](https://github.com/wycats/handlebars.js/blob/master/release-notes.md) - [Commits](handlebars-lang/handlebars.js@v4.1.2...v4.5.3) Signed-off-by: dependabot[bot] <[email protected]> * Fix displayName for cosmosDBTrigger (#399) * Update bindings.json Co-authored-by: [email protected] <Serverless Azure Functions> Co-authored-by: My <[email protected]> Co-authored-by: Wallace Breza <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ofek Bashan <[email protected]>

tbarlow12 commented Sep 30, 2019

View reviewed changes

src/services/resourceService.ts Show resolved Hide resolved

tbarlow12 closed this Sep 30, 2019

tbarlow12 reopened this Sep 30, 2019

tbarlow12 closed this Oct 1, 2019

tbarlow12 reopened this Oct 1, 2019

tbarlow12 closed this Oct 1, 2019

tbarlow12 reopened this Oct 1, 2019

mydiemho approved these changes Oct 1, 2019

View reviewed changes

src/services/resourceService.test.ts Show resolved Hide resolved

src/services/resourceService.test.ts Show resolved Hide resolved

wbreza approved these changes Oct 2, 2019

View reviewed changes

fix: Sort deployments in descending order and fix APIM arm template

b1779a7

tbarlow12 force-pushed the tabarlow/fix-apim-template branch from 00f495e to b1779a7 Compare October 3, 2019 17:32

tbarlow12 merged commit 865cfd0 into dev Oct 3, 2019

tbarlow12 deleted the tabarlow/fix-apim-template branch October 3, 2019 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Sort deployments in descending order and fix APIM arm template #354

fix: Sort deployments in descending order and fix APIM arm template #354

tbarlow12 commented Sep 30, 2019

mydiemho left a comment

wbreza left a comment

wbreza Oct 2, 2019

wbreza Oct 2, 2019

wbreza Oct 2, 2019

fix: Sort deployments in descending order and fix APIM arm template #354

fix: Sort deployments in descending order and fix APIM arm template #354

Conversation

tbarlow12 commented Sep 30, 2019

What did you implement:

How did you implement it:

How can we verify it:

Todos:

mydiemho left a comment

Choose a reason for hiding this comment

wbreza left a comment

Choose a reason for hiding this comment

wbreza Oct 2, 2019

Choose a reason for hiding this comment

wbreza Oct 2, 2019

Choose a reason for hiding this comment

wbreza Oct 2, 2019

Choose a reason for hiding this comment