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

Configure and deploy an Azure App Service #74

Merged
merged 22 commits into from
May 1, 2019

Conversation

manojvazirani
Copy link
Contributor

As a developer, I can configure and deploy Azure App Service. The App service provides

  • App/container hosting using app service
  • Public IP
  • Load balancing

All Submissions:


  • [YES] Have you followed the guidelines in our Contributing document?
  • [YES] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [YES] I have updated the documentation accordingly.
  • [NA] I have added tests to cover my changes.
  • [NA] All new and existing tests passed.
  • [YES] My code follows the code style of this project.
  • [NA] I ran lint checks locally prior to submission.
  • [YES] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

What is the current behavior?


Issue Number: #60

What is the new behavior?


  • Added a provider that configures Azure infrastructure for Terraform.
  • Added the App Service Plan by using data inputs from the user in Terraform
  • Added App Service by using data inputs from the user in Terraform
  • Added Public IP and Load Balancer to the App Service module which uses the above inputs provided by the user.

Does this introduce a breaking change?


  • [NO]

Any relevant logs, error output, etc?


(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Other information


This PR extends on top of PR #72

cobalt_appsvc

Copy link
Contributor

@erikschlegel erikschlegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to comments in PR 72 as they're related to this PR as well.

erikschlegel added a commit that referenced this pull request Apr 25, 2019
# This is the 1st commit message:

Adding docker test base image

# This is the commit message #2:

Adding test files

# This is the commit message #3:

testing

# This is the commit message #4:

Added folder

# This is the commit message #5:

Added folder

# This is the commit message #6:

Azure Devops CI Build Pipeline (#67)

* Adding azure devops build pipeline

* Adding README

* Adding azure build badge

# This is the commit message #7:

saving latest tests

# This is the commit message #8:

Adding docker test base image (#68)


# This is the commit message #9:

EOD checkin

# This is the commit message #10:

posting latest

# This is the commit message #11:

eod commits

# This is the commit message #12:

eod commit

# This is the commit message #13:

refactored test directory structure

# This is the commit message #14:

Refactored build

# This is the commit message #15:

Adding hello world azure simple app

# This is the commit message #16:

Adding hello world azure simple app

# This is the commit message #17:

refactoring test harness

# This is the commit message #18:

refactoring test harness

# This is the commit message #19:

checking latest

# This is the commit message #20:

committing latest

# This is the commit message #21:

checking in latest changes

# This is the commit message #22:

Checking in latest progress

# This is the commit message #23:

applying final touches before PR submission

# This is the commit message #24:

updating azure keyvault secret names

# This is the commit message #25:

Checking in latest progress

# This is the commit message #26:

pushing latest pipeline definition

# This is the commit message #27:

updating build pipeline variable format

# This is the commit message #28:

updating build pipeline variable format

# This is the commit message #29:

cleaning up build variables

# This is the commit message #30:

adding minor changes to azure build pipeline

# This is the commit message #31:

updating azure keyvault secret names

# This is the commit message #32:

updating azure keyvault secret names

# This is the commit message #33:

fixing docker login azdo step

# This is the commit message #34:

fixing docker login azdo step

# This is the commit message #35:

fixing docker login azdo step

# This is the commit message #36:

fixing docker login azdo step

# This is the commit message #37:

updating build pipeline variables

# This is the commit message #38:

updating build pipeline variables

# This is the commit message #39:

updating build pipeline variables

# This is the commit message #40:

updating build pipeline variables

# This is the commit message #41:

updating build pipeline variables

# This is the commit message #42:

updating build pipeline variables

# This is the commit message #43:

applying fix to git diff script

# This is the commit message #44:

applying fix to git diff script

# This is the commit message #45:

applying fix to git diff script

# This is the commit message #46:

applying fix to git diff script

# This is the commit message #47:

applying fix to git diff script

# This is the commit message #48:

resolving pipeline bug

# This is the commit message #49:

fixing build error

# This is the commit message #50:

fixing build error

# This is the commit message #51:

fixing build error

# This is the commit message #52:

fixing build error

# This is the commit message #53:

fixing build error

# This is the commit message #54:

fixing build error

# This is the commit message #55:

fixing build error

# This is the commit message #56:

fixing build error

# This is the commit message #57:

fixing build error

# This is the commit message #58:

fixing build error

# This is the commit message #59:

fixing build error

# This is the commit message #60:

fixing build error

# This is the commit message #61:

fixing build error

# This is the commit message #62:

fixing build error

# This is the commit message #63:

fixing build error

# This is the commit message #64:

fixing build error

# This is the commit message #65:

fixing build error

# This is the commit message #66:

fixing build error

# This is the commit message #67:

fixing build error

# This is the commit message #68:

fixing build error

# This is the commit message #69:

fixing build error

# This is the commit message #70:

fixing build error

# This is the commit message #71:

fixing build error

# This is the commit message #72:

fixing build error

# This is the commit message #73:

trying acr fix

# This is the commit message #74:

latest changes

# This is the commit message #75:

removing docker dependency on azure devops pipeline

# This is the commit message #76:

removing docker dependency on azure devops pipeline

# This is the commit message #77:

removing docker dependency on azure devops pipeline

# This is the commit message #78:

removing docker dependency on azure devops pipeline

# This is the commit message #79:

removing docker dependency on azure devops pipeline

# This is the commit message #80:

removing docker dependency on azure devops pipeline

# This is the commit message #81:

removing docker dependency on azure devops pipeline

# This is the commit message #82:

removing docker dependency on azure devops pipeline

# This is the commit message #83:

removing docker dependency on azure devops pipeline

# This is the commit message #84:

removing docker dependency on azure devops pipeline

# This is the commit message #85:

removing docker dependency on azure devops pipeline

# This is the commit message #86:

removing docker dependency on azure devops pipeline

# This is the commit message #87:

removing docker dependency on azure devops pipeline

# This is the commit message #88:

removing docker dependency on azure devops pipeline

# This is the commit message #89:

removing docker dependency on azure devops pipeline

# This is the commit message #90:

removing docker dependency on azure devops pipeline

# This is the commit message #91:

removing docker dependency on azure devops pipeline

# This is the commit message #92:

removing docker dependency on azure devops pipeline

# This is the commit message #93:

removing docker dependency on azure devops pipeline

# This is the commit message #94:

removing docker dependency on azure devops pipeline

# This is the commit message #95:

removing docker dependency on azure devops pipeline

# This is the commit message #96:

removing docker dependency on azure devops pipeline

# This is the commit message #97:

removing docker dependency on azure devops pipeline

# This is the commit message #98:

final commit including resource name randomization

# This is the commit message #99:

Add provider to configure az infra for Terraform
default = "eastus"
}

resource "azurerm_app_service" "appsvc" {
Copy link
Contributor

@erikschlegel erikschlegel Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documented usage should be aligned with how we're using these modules in our own templates. That being said, we should reference our own modules as terraform modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module is being defined here. And the module usage is defined below.

Copy link
Contributor

@erikschlegel erikschlegel Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the goal of these modules is for developers to avoid explicitly defining resource like this resource "azurerm_app_service" "appsvc"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the actual usage is after the definition. A couple more lines down, the example usage specifies how it has to be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my comment was not addressed. The documentation seems a bit confusing as this section represents how a developer would use the app_service module. Can you please remove the module definition from the readme and reference the reader to the location of the definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an usage documentation example for how the load balancer module is defined https://github.com/Azure/terraform-azurerm-loadbalancer

infra/modules/providers/azure/app-service/main.tf Outdated Show resolved Hide resolved
resource "azurerm_app_service" "appsvc" {
name = "${var.app_service_name}"
location = "${azurerm_resource_group.appsvc.location}"
resource_group_name = "${azurerm_resource_group.appsvc.name}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear, the resource group reference should be the one from the app service plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The referencing happens at azure-simple level as compared to module definition level as explained above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the variable name isazurerm_resource_group.appsvc.name which is misleading as the resource group name is sourced from the app service plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of reasons for this approach:

  1. The module app service will be consistent on the resource names.
  2. While debugging an issue, module reference name will always help since every module will have a reference name.
  3. If there are multiple resources referenced from previous modules, tracking back to where the resource came from will be easier for the user during deployment.

name = "${var.app_service_name}"
location = "${azurerm_resource_group.appsvc.location}"
resource_group_name = "${azurerm_resource_group.appsvc.name}"
app_service_plan_id = "${var.app_service_plan_id}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the app_settings and site_config are required for container deployment... How would a developer provide a container and DOCKER_REGISTRY_SERVER_URL to the provisioned app service resource?

erikschlegel added a commit that referenced this pull request Apr 25, 2019
* # This is a combination of 99 commits.
# This is the 1st commit message:

Adding docker test base image

# This is the commit message #2:

Adding test files

# This is the commit message #3:

testing

# This is the commit message #4:

Added folder

# This is the commit message #5:

Added folder

# This is the commit message #6:

Azure Devops CI Build Pipeline (#67)

* Adding azure devops build pipeline

* Adding README

* Adding azure build badge

# This is the commit message #7:

saving latest tests

# This is the commit message #8:

Adding docker test base image (#68)


# This is the commit message #9:

EOD checkin

# This is the commit message #10:

posting latest

# This is the commit message #11:

eod commits

# This is the commit message #12:

eod commit

# This is the commit message #13:

refactored test directory structure

# This is the commit message #14:

Refactored build

# This is the commit message #15:

Adding hello world azure simple app

# This is the commit message #16:

Adding hello world azure simple app

# This is the commit message #17:

refactoring test harness

# This is the commit message #18:

refactoring test harness

# This is the commit message #19:

checking latest

# This is the commit message #20:

committing latest

# This is the commit message #21:

checking in latest changes

# This is the commit message #22:

Checking in latest progress

# This is the commit message #23:

applying final touches before PR submission

# This is the commit message #24:

updating azure keyvault secret names

# This is the commit message #25:

Checking in latest progress

# This is the commit message #26:

pushing latest pipeline definition

# This is the commit message #27:

updating build pipeline variable format

# This is the commit message #28:

updating build pipeline variable format

# This is the commit message #29:

cleaning up build variables

# This is the commit message #30:

adding minor changes to azure build pipeline

# This is the commit message #31:

updating azure keyvault secret names

# This is the commit message #32:

updating azure keyvault secret names

# This is the commit message #33:

fixing docker login azdo step

# This is the commit message #34:

fixing docker login azdo step

# This is the commit message #35:

fixing docker login azdo step

# This is the commit message #36:

fixing docker login azdo step

# This is the commit message #37:

updating build pipeline variables

# This is the commit message #38:

updating build pipeline variables

# This is the commit message #39:

updating build pipeline variables

# This is the commit message #40:

updating build pipeline variables

# This is the commit message #41:

updating build pipeline variables

# This is the commit message #42:

updating build pipeline variables

# This is the commit message #43:

applying fix to git diff script

# This is the commit message #44:

applying fix to git diff script

# This is the commit message #45:

applying fix to git diff script

# This is the commit message #46:

applying fix to git diff script

# This is the commit message #47:

applying fix to git diff script

# This is the commit message #48:

resolving pipeline bug

# This is the commit message #49:

fixing build error

# This is the commit message #50:

fixing build error

# This is the commit message #51:

fixing build error

# This is the commit message #52:

fixing build error

# This is the commit message #53:

fixing build error

# This is the commit message #54:

fixing build error

# This is the commit message #55:

fixing build error

# This is the commit message #56:

fixing build error

# This is the commit message #57:

fixing build error

# This is the commit message #58:

fixing build error

# This is the commit message #59:

fixing build error

# This is the commit message #60:

fixing build error

# This is the commit message #61:

fixing build error

# This is the commit message #62:

fixing build error

# This is the commit message #63:

fixing build error

# This is the commit message #64:

fixing build error

# This is the commit message #65:

fixing build error

# This is the commit message #66:

fixing build error

# This is the commit message #67:

fixing build error

# This is the commit message #68:

fixing build error

# This is the commit message #69:

fixing build error

# This is the commit message #70:

fixing build error

# This is the commit message #71:

fixing build error

# This is the commit message #72:

fixing build error

# This is the commit message #73:

trying acr fix

# This is the commit message #74:

latest changes

# This is the commit message #75:

removing docker dependency on azure devops pipeline

# This is the commit message #76:

removing docker dependency on azure devops pipeline

# This is the commit message #77:

removing docker dependency on azure devops pipeline

# This is the commit message #78:

removing docker dependency on azure devops pipeline

# This is the commit message #79:

removing docker dependency on azure devops pipeline

# This is the commit message #80:

removing docker dependency on azure devops pipeline

# This is the commit message #81:

removing docker dependency on azure devops pipeline

# This is the commit message #82:

removing docker dependency on azure devops pipeline

# This is the commit message #83:

removing docker dependency on azure devops pipeline

# This is the commit message #84:

removing docker dependency on azure devops pipeline

# This is the commit message #85:

removing docker dependency on azure devops pipeline

# This is the commit message #86:

removing docker dependency on azure devops pipeline

# This is the commit message #87:

removing docker dependency on azure devops pipeline

# This is the commit message #88:

removing docker dependency on azure devops pipeline

# This is the commit message #89:

removing docker dependency on azure devops pipeline

# This is the commit message #90:

removing docker dependency on azure devops pipeline

# This is the commit message #91:

removing docker dependency on azure devops pipeline

# This is the commit message #92:

removing docker dependency on azure devops pipeline

# This is the commit message #93:

removing docker dependency on azure devops pipeline

# This is the commit message #94:

removing docker dependency on azure devops pipeline

# This is the commit message #95:

removing docker dependency on azure devops pipeline

# This is the commit message #96:

removing docker dependency on azure devops pipeline

# This is the commit message #97:

removing docker dependency on azure devops pipeline

# This is the commit message #98:

final commit including resource name randomization

# This is the commit message #99:

Add provider to configure az infra for Terraform

* resolving merge conflicts for squash

Updated documentation based on review comments

Integrate review comments for documentation.

Add usage example for each of the providers

Adding environment template descriptions

removing go version var from azdo pipeline as it's unused

removing unused setup folder

adding more verbose logging for azure devops failure

fixing pull request branch name resolution bug

fixing pull request branch name resolution bug

addressing nick's pr comments

documenting script and docker images

cleaning up required env var check

fixing base branch bug

Added service plan terraform templates to Azure

Adding tags for resources

Integrate review comments to add random id to name

Add tags, capacity and reserved for service plan

Minor Fixes for documentation

Integrate review comments related to resource naming

Remove Optional and Required in variables.tf

Update variables.tf

Update Service Plan Tier to Isolated

squaching commits

applied base image suggestions from nick

* squashing commits and slimming down the build image

* removing go* path from azure devops pipeline as we removed go mods

* refactoring our azure devops pipeline so it's aligned with the local test runner

* fixing azure devops pipeline bug

* fixing base image build variable reference bug

* Addressing nick's PR comments

variable "app_service_name" {
description = "The name of the app service to be created"
default = ["appsvc11", "appsvc12"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn’t have a default value.

count = "${length(var.app_service_name)}"

app_settings {
DOCKER_REGISTRY_SERVER_URL = "${var.docker_registry_server_url[count.index]}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels rigid as in most cases developers will be using the same docker registry. In the instance of deploying multiple app services, one would have to redefine the docker credentials each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be rigid, if we don't give developers the option. That is the reason each app-service has these settings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my point is that if you're using the same registry across 5 app services then someone is going to have to define the docker registry 5 times(all containing the same values)... Also, this seems like an unnecessary step for images hosted publicly on docker hub.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but a developer now has the option of adding it for an app service. If not they are optional and can be kept empty. Presuming that the docker registry is going to be same is against best practices which kind of takes away the option of flexibility for the developer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this was marked as resolved as this approach would force me to provide my docker credentials for public images hosted on docker hub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supporting multiple docker hosts is out of scope for this user story as the technical design assumes the customer is using a single docker registry(either local or public)

infra/modules/providers/azure/app-service/main.tf Outdated Show resolved Hide resolved
}

site_config {
linux_fx_version = "${var.site_config_linux_fx_version[count.index]}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these variables defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variables are defined in variables.tf file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both the app service name and container exec command should be driven off the same variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an ideal situation yes but that seems too rigid to push developers to give same names. Many containers will have additional versions that might not be part of the app service name.

Copy link
Contributor

@erikschlegel erikschlegel Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.... if we go with multiple variables, how are you accounting for situations where there's an array length mismatch between site_config_linux_fx_version and app_service_name variables?

description = "The tier under which the service plan is created. Details can be found at https://docs.microsoft.com/en-us/azure/app-service/overview-hosting-plans"
default = "Isolated"
description = "The tier under which the service plan is created. Details can be found at https://docs.microsoft.com/en-us/azure/app-service/overview-hosting-plans"
default = "Isolated"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not default to isolated as some developers will not need to run this on a private vnet. If a developer needs to run this in a vnet then we should provide a template to automate that and add this default in the relevant TF template(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we had discussed in the meeting on call with @code4clouds. I did not have a default value before but we agreed to add this default for Service plans inside vnet based on the Game plan document.

Copy link
Contributor

@erikschlegel erikschlegel Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed adding this default in the template variables.tf that stands up a vnet, not the module. Most users of this module will not need to run this in isolated mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a design decision that should be approved by @James-Nance @day-jeff . If we are building an Enterprise level App Reference then Isolated should be mandatory as this is the most secure way to protect the information flowing between the services. Enterprise App Reference in my head = PII, information that shouldn't be tampered or exposed outside of a VNET.

Copy link
Contributor

@erikschlegel erikschlegel Apr 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn’t a design decision as vnets are out of scope for this particular user story🙂. Generally speaking, we still need to do more research to double check whether isolated app service is required for private vnets / app services. This particular module will be used by developers for app service plans which don’t necessarily require a private vnet. Isolated app services may make sense in certain scenarios but are expensive Instance. This decision is specific to the deployment environment(template) as this module will be used for azure customer / developer(s) interested in deploying app services.

infra/modules/providers/azure/app-service/variables.tf Outdated Show resolved Hide resolved
default = "eastus"
}

resource "azurerm_app_service" "appsvc" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my comment was not addressed. The documentation seems a bit confusing as this section represents how a developer would use the app_service module. Can you please remove the module definition from the readme and reference the reader to the location of the definition.

default = "eastus"
}

resource "azurerm_app_service" "appsvc" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an usage documentation example for how the load balancer module is defined https://github.com/Azure/terraform-azurerm-loadbalancer

erikschlegel added a commit that referenced this pull request Apr 26, 2019
* # This is a combination of 99 commits.
# This is the 1st commit message:

Adding docker test base image

# This is the commit message #2:

Adding test files

# This is the commit message #3:

testing

# This is the commit message #4:

Added folder

# This is the commit message #5:

Added folder

# This is the commit message #6:

Azure Devops CI Build Pipeline (#67)

* Adding azure devops build pipeline

* Adding README

* Adding azure build badge

# This is the commit message #7:

saving latest tests

# This is the commit message #8:

Adding docker test base image (#68)


# This is the commit message #9:

EOD checkin

# This is the commit message #10:

posting latest

# This is the commit message #11:

eod commits

# This is the commit message #12:

eod commit

# This is the commit message #13:

refactored test directory structure

# This is the commit message #14:

Refactored build

# This is the commit message #15:

Adding hello world azure simple app

# This is the commit message #16:

Adding hello world azure simple app

# This is the commit message #17:

refactoring test harness

# This is the commit message #18:

refactoring test harness

# This is the commit message #19:

checking latest

# This is the commit message #20:

committing latest

# This is the commit message #21:

checking in latest changes

# This is the commit message #22:

Checking in latest progress

# This is the commit message #23:

applying final touches before PR submission

# This is the commit message #24:

updating azure keyvault secret names

# This is the commit message #25:

Checking in latest progress

# This is the commit message #26:

pushing latest pipeline definition

# This is the commit message #27:

updating build pipeline variable format

# This is the commit message #28:

updating build pipeline variable format

# This is the commit message #29:

cleaning up build variables

# This is the commit message #30:

adding minor changes to azure build pipeline

# This is the commit message #31:

updating azure keyvault secret names

# This is the commit message #32:

updating azure keyvault secret names

# This is the commit message #33:

fixing docker login azdo step

# This is the commit message #34:

fixing docker login azdo step

# This is the commit message #35:

fixing docker login azdo step

# This is the commit message #36:

fixing docker login azdo step

# This is the commit message #37:

updating build pipeline variables

# This is the commit message #38:

updating build pipeline variables

# This is the commit message #39:

updating build pipeline variables

# This is the commit message #40:

updating build pipeline variables

# This is the commit message #41:

updating build pipeline variables

# This is the commit message #42:

updating build pipeline variables

# This is the commit message #43:

applying fix to git diff script

# This is the commit message #44:

applying fix to git diff script

# This is the commit message #45:

applying fix to git diff script

# This is the commit message #46:

applying fix to git diff script

# This is the commit message #47:

applying fix to git diff script

# This is the commit message #48:

resolving pipeline bug

# This is the commit message #49:

fixing build error

# This is the commit message #50:

fixing build error

# This is the commit message #51:

fixing build error

# This is the commit message #52:

fixing build error

# This is the commit message #53:

fixing build error

# This is the commit message #54:

fixing build error

# This is the commit message #55:

fixing build error

# This is the commit message #56:

fixing build error

# This is the commit message #57:

fixing build error

# This is the commit message #58:

fixing build error

# This is the commit message #59:

fixing build error

# This is the commit message #60:

fixing build error

# This is the commit message #61:

fixing build error

# This is the commit message #62:

fixing build error

# This is the commit message #63:

fixing build error

# This is the commit message #64:

fixing build error

# This is the commit message #65:

fixing build error

# This is the commit message #66:

fixing build error

# This is the commit message #67:

fixing build error

# This is the commit message #68:

fixing build error

# This is the commit message #69:

fixing build error

# This is the commit message #70:

fixing build error

# This is the commit message #71:

fixing build error

# This is the commit message #72:

fixing build error

# This is the commit message #73:

trying acr fix

# This is the commit message #74:

latest changes

# This is the commit message #75:

removing docker dependency on azure devops pipeline

# This is the commit message #76:

removing docker dependency on azure devops pipeline

# This is the commit message #77:

removing docker dependency on azure devops pipeline

# This is the commit message #78:

removing docker dependency on azure devops pipeline

# This is the commit message #79:

removing docker dependency on azure devops pipeline

# This is the commit message #80:

removing docker dependency on azure devops pipeline

# This is the commit message #81:

removing docker dependency on azure devops pipeline

# This is the commit message #82:

removing docker dependency on azure devops pipeline

# This is the commit message #83:

removing docker dependency on azure devops pipeline

# This is the commit message #84:

removing docker dependency on azure devops pipeline

# This is the commit message #85:

removing docker dependency on azure devops pipeline

# This is the commit message #86:

removing docker dependency on azure devops pipeline

# This is the commit message #87:

removing docker dependency on azure devops pipeline

# This is the commit message #88:

removing docker dependency on azure devops pipeline

# This is the commit message #89:

removing docker dependency on azure devops pipeline

# This is the commit message #90:

removing docker dependency on azure devops pipeline

# This is the commit message #91:

removing docker dependency on azure devops pipeline

# This is the commit message #92:

removing docker dependency on azure devops pipeline

# This is the commit message #93:

removing docker dependency on azure devops pipeline

# This is the commit message #94:

removing docker dependency on azure devops pipeline

# This is the commit message #95:

removing docker dependency on azure devops pipeline

# This is the commit message #96:

removing docker dependency on azure devops pipeline

# This is the commit message #97:

removing docker dependency on azure devops pipeline

# This is the commit message #98:

final commit including resource name randomization

# This is the commit message #99:

Add provider to configure az infra for Terraform

* resolving merge conflicts for squash

Updated documentation based on review comments

Integrate review comments for documentation.

Add usage example for each of the providers

Adding environment template descriptions

removing go version var from azdo pipeline as it's unused

removing unused setup folder

adding more verbose logging for azure devops failure

fixing pull request branch name resolution bug

fixing pull request branch name resolution bug

addressing nick's pr comments

documenting script and docker images

cleaning up required env var check

fixing base branch bug

Added service plan terraform templates to Azure

Adding tags for resources

Integrate review comments to add random id to name

Add tags, capacity and reserved for service plan

Minor Fixes for documentation

Integrate review comments related to resource naming

Remove Optional and Required in variables.tf

Update variables.tf

Update Service Plan Tier to Isolated

squaching commits

applied base image suggestions from nick

* squashing commits and slimming down the build image

* removing go* path from azure devops pipeline as we removed go mods

* refactoring our azure devops pipeline so it's aligned with the local test runner

* fixing azure devops pipeline bug

* fixing base image build variable reference bug

* Addressing nick's PR comments

* resolving issue with failed builds after completing branches merges

* addressing a bug where test harness directory changes were not getting picked up by the harness

* removing local echo statement

* Adding directory exists check and app service container command variable

* addressing nick's PR comments

* adding git diff extension whitelist filter

```
module "service_plan" {
resource_group_name = "test-rg"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should reference TF variables ie "${var.resource_group_name}" and avoid hard coding variables when using modules.

count = "${length(var.app_service_name)}"

app_settings {
DOCKER_REGISTRY_SERVER_URL = "${var.docker_registry_server_url[count.index]}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supporting multiple docker hosts is out of scope for this user story as the technical design assumes the customer is using a single docker registry(either local or public)

variable "docker_registry_server_username" {
description = "The docker registry server username for app service to be created"
type = "list"
default = ["", ""]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you provide more clarity on the rationale for defaulting this to an enpty array of 2 elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as discussed in the meeting

@manojvazirani
Copy link
Contributor Author

@erikschlegel @code4clouds : The PR is up for review again.

  • With respect to the open question that do we need a resource group name for service plan, the answer is yes since same service plan name can be in multiple resource groups.
  • So depends_on is not needed since service plan and resource group name is needed before deployment of app services which is now mentioned clearly in the documentation.

erikschlegel added a commit that referenced this pull request Apr 30, 2019
* # This is a combination of 99 commits.
# This is the 1st commit message:

Adding docker test base image

# This is the commit message #2:

Adding test files

# This is the commit message #3:

testing

# This is the commit message #4:

Added folder

# This is the commit message #5:

Added folder

# This is the commit message #6:

Azure Devops CI Build Pipeline (#67)

* Adding azure devops build pipeline

* Adding README

* Adding azure build badge

# This is the commit message #7:

saving latest tests

# This is the commit message #8:

Adding docker test base image (#68)


# This is the commit message #9:

EOD checkin

# This is the commit message #10:

posting latest

# This is the commit message #11:

eod commits

# This is the commit message #12:

eod commit

# This is the commit message #13:

refactored test directory structure

# This is the commit message #14:

Refactored build

# This is the commit message #15:

Adding hello world azure simple app

# This is the commit message #16:

Adding hello world azure simple app

# This is the commit message #17:

refactoring test harness

# This is the commit message #18:

refactoring test harness

# This is the commit message #19:

checking latest

# This is the commit message #20:

committing latest

# This is the commit message #21:

checking in latest changes

# This is the commit message #22:

Checking in latest progress

# This is the commit message #23:

applying final touches before PR submission

# This is the commit message #24:

updating azure keyvault secret names

# This is the commit message #25:

Checking in latest progress

# This is the commit message #26:

pushing latest pipeline definition

# This is the commit message #27:

updating build pipeline variable format

# This is the commit message #28:

updating build pipeline variable format

# This is the commit message #29:

cleaning up build variables

# This is the commit message #30:

adding minor changes to azure build pipeline

# This is the commit message #31:

updating azure keyvault secret names

# This is the commit message #32:

updating azure keyvault secret names

# This is the commit message #33:

fixing docker login azdo step

# This is the commit message #34:

fixing docker login azdo step

# This is the commit message #35:

fixing docker login azdo step

# This is the commit message #36:

fixing docker login azdo step

# This is the commit message #37:

updating build pipeline variables

# This is the commit message #38:

updating build pipeline variables

# This is the commit message #39:

updating build pipeline variables

# This is the commit message #40:

updating build pipeline variables

# This is the commit message #41:

updating build pipeline variables

# This is the commit message #42:

updating build pipeline variables

# This is the commit message #43:

applying fix to git diff script

# This is the commit message #44:

applying fix to git diff script

# This is the commit message #45:

applying fix to git diff script

# This is the commit message #46:

applying fix to git diff script

# This is the commit message #47:

applying fix to git diff script

# This is the commit message #48:

resolving pipeline bug

# This is the commit message #49:

fixing build error

# This is the commit message #50:

fixing build error

# This is the commit message #51:

fixing build error

# This is the commit message #52:

fixing build error

# This is the commit message #53:

fixing build error

# This is the commit message #54:

fixing build error

# This is the commit message #55:

fixing build error

# This is the commit message #56:

fixing build error

# This is the commit message #57:

fixing build error

# This is the commit message #58:

fixing build error

# This is the commit message #59:

fixing build error

# This is the commit message #60:

fixing build error

# This is the commit message #61:

fixing build error

# This is the commit message #62:

fixing build error

# This is the commit message #63:

fixing build error

# This is the commit message #64:

fixing build error

# This is the commit message #65:

fixing build error

# This is the commit message #66:

fixing build error

# This is the commit message #67:

fixing build error

# This is the commit message #68:

fixing build error

# This is the commit message #69:

fixing build error

# This is the commit message #70:

fixing build error

# This is the commit message #71:

fixing build error

# This is the commit message #72:

fixing build error

# This is the commit message #73:

trying acr fix

# This is the commit message #74:

latest changes

# This is the commit message #75:

removing docker dependency on azure devops pipeline

# This is the commit message #76:

removing docker dependency on azure devops pipeline

# This is the commit message #77:

removing docker dependency on azure devops pipeline

# This is the commit message #78:

removing docker dependency on azure devops pipeline

# This is the commit message #79:

removing docker dependency on azure devops pipeline

# This is the commit message #80:

removing docker dependency on azure devops pipeline

# This is the commit message #81:

removing docker dependency on azure devops pipeline

# This is the commit message #82:

removing docker dependency on azure devops pipeline

# This is the commit message #83:

removing docker dependency on azure devops pipeline

# This is the commit message #84:

removing docker dependency on azure devops pipeline

# This is the commit message #85:

removing docker dependency on azure devops pipeline

# This is the commit message #86:

removing docker dependency on azure devops pipeline

# This is the commit message #87:

removing docker dependency on azure devops pipeline

# This is the commit message #88:

removing docker dependency on azure devops pipeline

# This is the commit message #89:

removing docker dependency on azure devops pipeline

# This is the commit message #90:

removing docker dependency on azure devops pipeline

# This is the commit message #91:

removing docker dependency on azure devops pipeline

# This is the commit message #92:

removing docker dependency on azure devops pipeline

# This is the commit message #93:

removing docker dependency on azure devops pipeline

# This is the commit message #94:

removing docker dependency on azure devops pipeline

# This is the commit message #95:

removing docker dependency on azure devops pipeline

# This is the commit message #96:

removing docker dependency on azure devops pipeline

# This is the commit message #97:

removing docker dependency on azure devops pipeline

# This is the commit message #98:

final commit including resource name randomization

# This is the commit message #99:

Add provider to configure az infra for Terraform

* resolving merge conflicts for squash

Updated documentation based on review comments

Integrate review comments for documentation.

Add usage example for each of the providers

Adding environment template descriptions

removing go version var from azdo pipeline as it's unused

removing unused setup folder

adding more verbose logging for azure devops failure

fixing pull request branch name resolution bug

fixing pull request branch name resolution bug

addressing nick's pr comments

documenting script and docker images

cleaning up required env var check

fixing base branch bug

Added service plan terraform templates to Azure

Adding tags for resources

Integrate review comments to add random id to name

Add tags, capacity and reserved for service plan

Minor Fixes for documentation

Integrate review comments related to resource naming

Remove Optional and Required in variables.tf

Update variables.tf

Update Service Plan Tier to Isolated

squaching commits

applied base image suggestions from nick

* squashing commits and slimming down the build image

* removing go* path from azure devops pipeline as we removed go mods

* refactoring our azure devops pipeline so it's aligned with the local test runner

* fixing azure devops pipeline bug

* fixing base image build variable reference bug

* Addressing nick's PR comments

* resolving issue with failed builds after completing branches merges

* addressing a bug where test harness directory changes were not getting picked up by the harness

* removing local echo statement

* Adding directory exists check and app service container command variable

* addressing nick's PR comments

* adding git diff extension whitelist filter

* Overhauling main project readme

* Adding james to the readme

* Update README.md

* Updating readme from the test-harness directory
Copy link
Contributor

@erikschlegel erikschlegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks!

Build, deploy, and scale enterprise-grade web, mobile, and API apps running on any platform. Meet rigorous performance, scalability, security and compliance requirements while using a fully managed platform to perform infrastructure maintenance.

More information for Azure App Services can be found [here](https://azure.microsoft.com/en-us/services/app-service/)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General note, please update the usage section of the readme for the service plan module so that cobalt users have more context on how to define and configure there module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me open a new PR with the documentation changes for service plan.

@manojvazirani manojvazirani merged commit 2c993ab into master May 1, 2019
@manojvazirani manojvazirani deleted the mavazira/features/appsvc branch May 1, 2019 14:35
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.

None yet

3 participants