Repository: serverless/serverless-azure-functions
Pull request #358 (2019-10-07 21:04:14)

feat: Resource group tagging
<!--
1. Please check out and follow our Contributing Guidelines: https://github.com/serverless/serverless-azure-functions/CONTRIBUTING.md
2. Do not remove any section of the template. If something is not applicable leave it empty but leave it in the PR
3. Please follow the template, otherwise we'll have to ask you to update it and it will take longer until your PR is merged
-->

## What did you implement:

Tagging for the Azure resource group as defined within the `provider`. Example

```yaml
...
provider:
    tags:
        TAG_1: tag1Value
        TAG_2: tag2Value
```

## How did you implement it:

- Check for existing resource group
- Merge tags from existing with tags from provider config
- Add tags to `createOrUpdate` call of resource group

## How can we verify it:

- Deploy a service with `tags` defined in provider config

OR

- Manually define tag for resource group in Azure portal
- Deploy service
- Tag will not be lost

<!--
Add any applicable config, commands, screenshots or other resources
to make it easy for us to verify this works. The easier you make it for us
to review a PR, the faster we can review and merge it.

Examples:
* serverless.yml - Fully functioning to easily deploy changes
* Screenshots - Showing the difference between your output and the master
* Cloud Configuration - List cloud resources and show that the correct configuration is in place
* Other - Anything else that comes to mind to help us evaluate
-->

## Todos:

_**Note: Run `npm run test:ci` to run all validation checks on proposed changes**_

- [x] 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`_
- [x] Write tests and confirm existing functionality is not broken.  
       **Validate via `npm test`**
- [x] Write documentation
- [x] Provide verification config / commands / resources
- [x] Enable "Allow edits from maintainers" for this PR
- [x] Update the messages below

**_Is this ready for review?:_** YES
**_Is it a breaking change?:_** NO


codecov[bot]: # [Codecov](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=h1) Report
> Merging [#358](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=desc) into [dev](https://codecov.io/gh/serverless/serverless-azure-functions/commit/673008b6cc74f67834cac46a1bdc7e756356b6b9?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `83.33%`.

[![Impacted file tree graph](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358/graphs/tree.svg?width=650&token=ro919H5eps&height=150&src=pr)](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##              dev    #358      +/-   ##
=========================================
- Coverage   94.44%   94.4%   -0.04%     
=========================================
  Files          48      48              
  Lines        1782    1788       +6     
  Branches      280     281       +1     
=========================================
+ Hits         1683    1688       +5     
- Misses         98      99       +1     
  Partials        1       1
```


| [Impacted Files](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/services/resourceService.ts](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358/diff?src=pr&el=tree#diff-c3JjL3NlcnZpY2VzL3Jlc291cmNlU2VydmljZS50cw==) | `94.11% <83.33%> (-1.44%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=footer). Last update [673008b...6f564ae](https://codecov.io/gh/serverless/serverless-azure-functions/pull/358?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md
index f5ef0e9..b9101e9 100644
--- a/docs/DEPLOY.md
+++ b/docs/DEPLOY.md
@@ -14,6 +14,20 @@ By default, all functions are deploy to a stage (`dev`) and a region. The autoge
 - Create/Update app service plan, app service
 - Zipdeploy code
 
+### Tagging resource group
+
+To apply tags to your resource group, you can specify them in your provider config as such:
+
+```yaml
+...
+provider:
+  tags:
+    TAG_1: tagValue1
+    TAG_2: tagValue2
+```
+
+Existing tags with the same names will be updated. Existing tags (with different names) will **not** be removed.
+
 ### No resource group specified
 
 `sls deploy -s dev -r westus2`
@@ -41,7 +55,7 @@ then user try to deploy
 `sls deploy -s prod -r westus`
 
 1. Do we use the environment variables / creds associated with `dev` or `prod`?
-1. Someone new to the codecase see both the `serverless.yaml` and the command in a CD script, how can they tell which resourceGroup goes where.
+2. Someone new to the codecase see both the `serverless.yaml` and the command in a CD script, how can they tell which resourceGroup goes where.
 
 #### Specifying resourceGroup in command line
 
diff --git a/src/models/serverless.ts b/src/models/serverless.ts
index 16dfbe6..00393f0 100644
--- a/src/models/serverless.ts
+++ b/src/models/serverless.ts
@@ -40,6 +40,9 @@ export interface ServerlessAzureProvider {
   environment?: {
     [key: string]: any;
   };
+  tags?: {
+    [tagName: string]: string;
+  };
   deployment?: DeploymentConfig;
   deploymentName?: string;
   resourceGroup?: string;
diff --git a/src/services/resourceService.ts b/src/services/resourceService.ts
index 264aa8e..3973255 100644
--- a/src/services/resourceService.ts
+++ b/src/services/resourceService.ts
@@ -4,7 +4,7 @@ import { BaseService } from "./baseService";
 import { Utils } from "../shared/utils";
 import { AzureNamingService } from "./namingService";
 import { ArmDeployment, ArmTemplateProvisioningState } from "../models/armTemplates";
-import { DeploymentExtended } from "@azure/arm-resources/esm/models";
+import { DeploymentExtended, ResourceGroupsGetResponse } from "@azure/arm-resources/esm/models";
 
 export class ResourceService extends BaseService {
   private resourceClient: ResourceManagementClient;
@@ -87,12 +87,26 @@ export class ResourceService extends BaseService {
 
   public async deployResourceGroup() {
     this.log(`Creating resource group: ${this.resourceGroup}`);
+    
+    const resourceGroup = await this.getResourceGroup();
 
     return await this.resourceClient.resourceGroups.createOrUpdate(this.resourceGroup, {
       location: AzureNamingService.getNormalizedRegionName(this.configService.getRegion()),
+      tags: {
+        ...((resourceGroup) ? resourceGroup.tags : undefined),
+        ...this.config.provider.tags
+      }
     });
   }
 
+  public async getResourceGroup(): Promise<ResourceGroupsGetResponse> {
+    try {
+      return await this.resourceClient.resourceGroups.get(this.resourceGroup);
+    } catch (e) {
+      return undefined;
+    }
+  }
+
   public async deleteDeployment() {
     this.log(`Deleting deployment: ${this.deploymentName}`);
     return await this.resourceClient.deployments.deleteMethod(this.resourceGroup, this.deploymentName);


Test patch:
diff --git a/src/services/resourceService.test.ts b/src/services/resourceService.test.ts
index 9482e83..554d5df 100644
--- a/src/services/resourceService.test.ts
+++ b/src/services/resourceService.test.ts
@@ -18,6 +18,7 @@ describe("Resource Service", () => {
     ResourceManagementClient.prototype.resourceGroups = {
       createOrUpdate: jest.fn(),
       deleteMethod: jest.fn(),
+      get: jest.fn(),
     } as any;
 
     ResourceManagementClient.prototype.deployments = {
@@ -40,7 +41,7 @@ describe("Resource Service", () => {
     expect(() => new ResourceService(sls, options)).not.toThrowError();
   });
 
-  it("deploys a resource group", () => {
+  it("deploys a resource group", async () => {
     const sls = MockFactory.createTestServerless();
     const resourceGroup = "myResourceGroup"
     const location = "West Us";
@@ -50,13 +51,87 @@ describe("Resource Service", () => {
     sls.variables["azureCredentials"] = "fake credentials"
     const options = MockFactory.createTestServerlessOptions();
     const service = new ResourceService(sls, options);
-    service.deployResourceGroup();
+    await service.deployResourceGroup();
 
     expect(ResourceManagementClient.prototype.resourceGroups.createOrUpdate)
-      .toBeCalledWith(resourceGroup, { location: expectedLocation });
+      .toBeCalledWith(resourceGroup, {
+        location: expectedLocation,
+        tags: {}
+      });
   });
 
-  it("deletes a deployment", () => {
+  it("deploys a resource group with tags", async () => {
+    const tags = {
+      TAG_1: "tag 1 value",
+      TAG_2: "tag 2 value"
+    }
+    const resourceGroup = "resourceGroup";
+    const location = "West Us";
+    const expectedLocation = AzureNamingService.getNormalizedRegionName(location);
+    
+    const sls = MockFactory.createTestServerless();
+    sls.service.provider["resourceGroup"] = resourceGroup
+    sls.service.provider.region = location;
+    sls.service.provider["tags"] = tags;
+    
+    const service = new ResourceService(sls, {} as any);
+
+    await service.deployResourceGroup();
+    expect(ResourceManagementClient.prototype.resourceGroups.createOrUpdate)
+      .toBeCalledWith(resourceGroup, {
+        location: expectedLocation,
+        tags
+      });
+  });
+
+  it("deploys a resource group with existing tags", async () => {
+    const existingTags = {
+      TAG_2: "2",
+      TAG_3: "3",
+    }
+
+    ResourceManagementClient.prototype.resourceGroups = {
+      createOrUpdate: jest.fn(),
+      deleteMethod: jest.fn(),
+      get: jest.fn(() => Promise.resolve({
+        tags: {
+          ...existingTags
+        }
+      })),
+    } as any;
+
+    const tags = {
+      TAG_1: "tag 1 value",
+      TAG_2: "tag 2 value"
+    }
+    const resourceGroup = "resourceGroup";
+    const location = "West Us";
+    const expectedLocation = AzureNamingService.getNormalizedRegionName(location);
+    
+    const sls = MockFactory.createTestServerless();
+    sls.service.provider["resourceGroup"] = resourceGroup
+    sls.service.provider.region = location;
+    sls.service.provider["tags"] = tags;
+    
+    const service = new ResourceService(sls, {} as any);
+
+    const expectedTags = {
+      TAG_1: "tag 1 value",
+      TAG_2: "tag 2 value",
+      TAG_3: "3",
+    }
+
+    await service.deployResourceGroup();
+    expect(ResourceManagementClient.prototype.resourceGroups.createOrUpdate)
+      .toBeCalledWith(resourceGroup, {
+        location: expectedLocation,
+        tags: {
+          ...expectedTags
+        }
+      });
+  });
+
+  it("deletes a deployment", async () => {
     const sls = MockFactory.createTestServerless();
     const resourceGroup = "myResourceGroup";
     const deploymentName = "myDeployment";
@@ -65,21 +140,21 @@ describe("Resource Service", () => {
     sls.variables["azureCredentials"] = "fake credentials"
     const options = MockFactory.createTestServerlessOptions();
     const service = new ResourceService(sls, options);
-    service.deleteDeployment();
+    await service.deleteDeployment();
     const call = (ResourceManagementClient.prototype.deployments.deleteMethod as any).mock.calls[0];
     expect(call[0]).toEqual(resourceGroup);
     const expectedDeploymentNameRegex = new RegExp(deploymentName + "-t([0-9]+)")
     expect(call[1]).toMatch(expectedDeploymentNameRegex)
   });
 
-  it("deletes a resource group", () => {
+  it("deletes a resource group", async () => {
     const sls = MockFactory.createTestServerless();
     const resourceGroup = "myResourceGroup";
     sls.service.provider["resourceGroup"] = resourceGroup
     sls.variables["azureCredentials"] = "fake credentials"
     const options = MockFactory.createTestServerlessOptions();
     const service = new ResourceService(sls, options);
-    service.deleteResourceGroup();
+    await service.deleteResourceGroup();
     expect(ResourceManagementClient.prototype.resourceGroups.deleteMethod)
       .toBeCalledWith(resourceGroup);
   });
@@ -143,7 +218,7 @@ describe("Resource Service", () => {
     expect(deploymentString).toEqual(expectedDeploymentString);
   });
 
-  it("gets deployment template",async () => {
+  it("gets deployment template", async () => {
     const sls = MockFactory.createTestServerless();
     const resourceGroup = "myResourceGroup";
     sls.service.provider["resourceGroup"] = resourceGroup