Enable source control management as option for user to deploy web apps #826

metacpp · 2018-02-13T02:43:14Z

This PR includes:

New argument, scm_type, as deployment option for web app. Supported value list include "None" and "LocalGit", will add more options in a later time.
New site_source_control_props with necessary git repo and branch information.

tombuildsstuff

Thanks for this PR - I've taken a look through and this mostly looks good to me; it'd be good to expose the value of the Source Control URL, so that folks could use this value without having to go to the Portal first; so can we look into adding that?

Thanks!

tombuildsstuff · 2018-02-13T14:46:11Z

website/docs/r/app_service.html.markdown

@@ -160,6 +162,8 @@ The following arguments are supported:

 * `websockets_enabled` - (Optional) Should WebSockets be enabled?

+* `scm_type` - (Optional) The Source Control Management Type. Possible values are `None` and `LocalGit`. Defaults to `None`.


I'd suggest we change this to:

* `scm_type` - (Optional) The type of Source Control enabled for this App Service. Possible values include `None` and `LocalGit`. Defaults to `None`

and add a blue info box below it - stating the other types are coming soon

-> **Note:** Additional Source Control types will be added in the future, once support for them has been added in the Azure SDK for Go.

Good suggestion.

tombuildsstuff · 2018-02-13T15:15:24Z

azurerm/resource_arm_app_service.go

+								string(web.ScmTypeNone),
+								string(web.ScmTypeLocalGit),
+							}, false),
+						},


so it'd also be good to expose the value of the SCM URL as a computed field - which I believe should be returned from the GetSourceControl API call when this is set to ScmTypeLocalGit?

Thanks for the reminding, this is exactly what I did the recent commit, please see changes.

metacpp · 2018-02-14T01:13:17Z

@tombuildsstuff I've updated the PR to include the repo url and branch name.

tombuildsstuff

adding pending comments which have since been fixed

tombuildsstuff · 2018-02-14T00:30:10Z

azurerm/resource_arm_app_service.go

+
+			"site_source_control_props": {
+				Type:     schema.TypeList,
+				Optional: true,


given this isn't user-settable - we can remove the Optional assignment here (Computed is read-only)

tombuildsstuff · 2018-02-14T00:32:09Z

azurerm/resource_arm_app_service.go

@@ -239,6 +248,23 @@ func resourceArmAppService() *schema.Resource {
 				Type:     schema.TypeString,
 				Computed: true,
 			},
+
+			"site_source_control_props": {


can we rename this source_control (to match the other fields)

tombuildsstuff · 2018-02-14T03:22:45Z

azurerm/resource_arm_app_service.go

+
+			"source_control": {
+				Type:     schema.TypeList,
+				Optional: true,


can we remove this Optional assignment? it's only needed for user specifiable fields (and not readonly ones)

Good catch.

tombuildsstuff · 2018-02-14T03:24:09Z

website/docs/r/app_service.html.markdown

+`source_control` supports the following:
+
+* `repo_url` - URL of the Git repo.
+* `branch` - Branch name of the Git repo.


can we suffix both of these with .. the Git repository for this App Service

tombuildsstuff

hey @metacpp

Thanks for pushing those latest updates - if we can remove the Optional field assignment this otherwise looks good to me.

Thanks!

JunyiYi · 2018-02-14T18:48:21Z

azurerm/resource_arm_app_service_test.go

+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+  name = "acctestRG-%d"
+  location = "%s"


Aligning at '='?

Good catch.

JunyiYi · 2018-02-14T18:48:41Z

azurerm/resource_arm_app_service_test.go

+  app_service_plan_id = "${azurerm_app_service_plan.test.id}"
+
+  site_config {
+    scm_type          = "LocalGit"


Reduce spaces here?

Good catch.

metacpp · 2018-02-14T20:39:49Z

@tombuildsstuff the PR has been updated, please check.

tombuildsstuff

LGTM 👍 I'll kick off the test suite now

metacpp · 2018-02-15T18:32:40Z

@tombuildsstuff Thanks for the reviewing.

tombuildsstuff · 2018-02-15T19:34:26Z

@metacpp given App Service Slots shares the same Set function - it appears we need to add the field to that schema too:

------- Stdout: -------
=== RUN   TestAccAzureRMAppServiceSlot_importBasic
--- FAIL: TestAccAzureRMAppServiceSlot_importBasic (51.57s)
    testing.go:459: Step 0 error: Error applying: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: Invalid address to set: []string{"site_config", "0", "scm_type"}
    testing.go:519: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Error refreshing: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: azurerm_app_service_slot.test: Invalid address to set: []string{"site_config", "0", "scm_type"}
        
        State: <nil>
FAIL

I'll push a commit for this today

the tests reveal this requires a fix to the App Service Slot schema

metacpp · 2018-03-21T00:12:10Z

@tombuildsstuff I reopened this PR coz I worked with App Service team have the LocalGit deployment supported.

tombuildsstuff · 2018-03-28T13:09:54Z

azurerm/resource_arm_app_service.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"user_name": {


we'd tend to write this as username - I'll push a commit to make this consistent

Thanks for the updating.

tombuildsstuff

One minor comment (which I'll push a commit for) - but this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-03-28T14:00:47Z

App Service tests pass:

ghost · 2020-03-31T15:19:17Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

resolve conflict.

21db11c

metacpp requested review from tombuildsstuff, WodansSon and JunyiYi February 13, 2018 02:43

metacpp added the enhancement label Feb 13, 2018

metacpp added this to the 1.1.2 milestone Feb 13, 2018

metacpp added the service/app-service label Feb 13, 2018

Add default value for scm_type.

78f5d92

tombuildsstuff suggested changes Feb 13, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Feb 13, 2018

add site_source_control_props as attributes.

e0cd2e8

Change the naming to simpler one and update documentation.

180fd76

tombuildsstuff reviewed Feb 14, 2018

View reviewed changes

JunyiYi reviewed Feb 14, 2018

View reviewed changes

metacpp added 2 commits February 14, 2018 11:49

Update the documentation and code style.

234b56f

Update the test case and schema for source_control.

834c914

tombuildsstuff previously approved these changes Feb 15, 2018

View reviewed changes

tombuildsstuff removed the waiting-response label Feb 15, 2018

tombuildsstuff self-assigned this Feb 15, 2018

metacpp added 3 commits February 15, 2018 14:25

resolve conflict.

749960e

Add default value for scm_type.

6437f64

add site_source_control_props as attributes.

0765cf6

metacpp added 4 commits March 16, 2018 17:30

add expand and flattern for publishing user.

ad47fdb

Add update and read for publishing_user.

2d0fbf8

Refine the code, test and documentation to support local git deployment.

d0d7ab5

Merge branch 'master' into hack_scm

1ba5381

metacpp reopened this Mar 21, 2018

metacpp modified the milestones: Future, 1.3.1, 1.4.0 Mar 21, 2018

WodansSon approved these changes Mar 21, 2018

View reviewed changes

JunyiYi approved these changes Mar 21, 2018

View reviewed changes

metacpp added goodformerge and removed goodformerge labels Mar 22, 2018

metacpp added 2 commits March 26, 2018 15:36

Update the schema for site_credential.password to be sensitive.

4860923

Merge branch 'master' into hack_scm

2efd09c

tombuildsstuff removed the good for merge label Mar 28, 2018

Renaming user_name -> username

43b5b49

tombuildsstuff reviewed Mar 28, 2018

View reviewed changes

tombuildsstuff approved these changes Mar 28, 2018

View reviewed changes

tombuildsstuff removed the upstream label Mar 28, 2018

tombuildsstuff merged commit 7020f3e into master Mar 28, 2018

tombuildsstuff deleted the hack_scm branch March 28, 2018 14:01

tombuildsstuff added a commit that referenced this pull request Mar 28, 2018

Updating to include #826

771f179

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable source control management as option for user to deploy web apps #826

Enable source control management as option for user to deploy web apps #826

metacpp commented Feb 13, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Feb 13, 2018

metacpp Feb 14, 2018

tombuildsstuff Feb 13, 2018

metacpp Feb 14, 2018

metacpp commented Feb 14, 2018

tombuildsstuff left a comment

tombuildsstuff Feb 14, 2018

metacpp Feb 14, 2018

tombuildsstuff Feb 14, 2018

metacpp Feb 14, 2018

tombuildsstuff Feb 14, 2018

metacpp Feb 14, 2018

tombuildsstuff Feb 14, 2018

metacpp Feb 14, 2018

tombuildsstuff left a comment

JunyiYi Feb 14, 2018

metacpp Feb 14, 2018

JunyiYi Feb 14, 2018

metacpp Feb 14, 2018

metacpp commented Feb 14, 2018

tombuildsstuff left a comment

metacpp commented Feb 15, 2018

tombuildsstuff commented Feb 15, 2018

metacpp commented Mar 21, 2018

tombuildsstuff Mar 28, 2018

metacpp Mar 28, 2018

tombuildsstuff left a comment

tombuildsstuff commented Mar 28, 2018

ghost commented Mar 31, 2020

		@@ -160,6 +162,8 @@ The following arguments are supported:

		* `websockets_enabled` - (Optional) Should WebSockets be enabled?

		* `scm_type` - (Optional) The Source Control Management Type. Possible values are `None` and `LocalGit`. Defaults to `None`.

Enable source control management as option for user to deploy web apps #826

Enable source control management as option for user to deploy web apps #826

Conversation

metacpp commented Feb 13, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp commented Feb 14, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp commented Feb 14, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

metacpp commented Feb 15, 2018

tombuildsstuff commented Feb 15, 2018

metacpp commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 28, 2018

ghost commented Mar 31, 2020

metacpp commented Feb 13, 2018 •

edited

Loading