[WIP]Add support of GitHub as deployment option by metacpp · Pull Request #1121 · hashicorp/terraform-provider-azurerm

metacpp · 2018-04-14T05:42:00Z

Still work in progress

Support GitHub as deployment option for azure web app, the workflow will be:

Generate Personal Access Token (read permission in both org and repo) to authorize Web App to access repo.
Set SCM Type as GitHub.
Set URL of repo and branch.

Will add example usage and tests in this PR very soon.

1. Add update operation for source_control and external_scm_credential. 2. Refactor the code logic of expand and flatten functions.

metacpp · 2018-04-14T05:45:05Z

This PR is used to resolve #1104 , which is opened by @istairbn

tombuildsstuff

hey @metacpp

I've taken a look through and left some comments in-line - before proceeding any further (e.g. tests/docs etc) with this PR we should focus on working out how the schema needs to look, in particular I don't believe we need the external_scm_credentials block at all since it can be in-lined into the VCS URI?

Thanks!

tombuildsstuff · 2018-04-14T07:47:52Z

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


neither of these fields can currently be specified by the user, so there's no need to make this optional at the moment?

In GitHub scenario, you need to specify the repo URL and branch name for Azure Web App to consume. It's not the same scenario as LocalGit, which will automatically generate the SCM URL hosted on Azure web app. I will update the example usage for GitHub scenario, then you will know what's the workflow.

tombuildsstuff · 2018-04-14T07:49:24Z

 				},
 			},
+
+			"external_scm_credential": {


I don't believe we don't need a separate block for this - as mentioned in this comment it can be part of the Repository URI in the source_control block, which will cover all cases. As such - can we remove this?

This is to allow end-user to set personal access token which will authorize Azure web app to access repo and org information through API way. In other words, this is the token required by Azure to integrate with any 3rd party deployment options.

We may have a better naming here, what's your suggestion ?

tombuildsstuff · 2018-04-14T07:49:57Z

+						"name": {
+							Type:     schema.TypeString,
+							Computed: true,
+							Default:  "GitHub",


in cases such as this (where this is within an Optional sub-object) - we can make this Required. We should also remove the Default value and Computed here, since it needs to be user specified

tombuildsstuff · 2018-04-16T15:55:24Z

 }

-func flattenAppServiceSiteCredential(input *web.UserProperties) []interface{} {
+func flattenAppServiceSiteCredential(input *web.User) []interface{} {


we should be pulling from the UserProperties object since the non-Properties object isn't actually valid in Azure (e.g. it doesn't actually match the schema and can be nil in older configurations)

User is using embedding with UserProperties, you can directly access any field in User object.

tombuildsstuff · 2018-04-16T15:55:49Z

+	return scmRet
+}
+
+func flattenAppServiceSourceControl(input *web.SiteSourceControl) []interface{} {


we should be pulling from the SiteSourceControlProperties object since the non-Properties object isn't actually valid in Azure (e.g. it doesn't actually match the schema and can be nil in older configurations)

Internal Go SDK is using SiteSourceControl as returned type and SiteSourceControlProperties is embedded class, which means that you can access all the fields in SiteSourceControl. It's meaningless to do any kind of de-referencing before flattening.

I don't understand what's your concern here, maybe a more concrete example ?

at this point in time, from memory that representation has changed in the past such that the top level can be nil vs the properties object being populated - that's not how it's represented in the JSON Response so we should match that where possible (which we do throughout the provider)

There's no any API return SiteSourceControlProperties, instead, it's SiteSourceControl.
If SiteSourceControl is nil, then you will not be able to get SiteSourceControlProperties, since they're the same object but different layer.

metacpp · 2018-04-16T20:14:53Z

@tombuildsstuff external_scm_credential is personal access token used by azure behind the scene to access org and repo information. It's not like the azure its own repo, there's no chance for any CI pipeline to directly access the repo hosted on GitHub. It's completely different scenario.

metacpp · 2018-05-02T21:41:01Z

This PR does have dependencies on #2910 and #1661.

metacpp · 2018-05-02T21:43:17Z

And for the personal token type, we have weak dependency on #2946

…trol.

tombuildsstuff · 2018-06-19T08:15:47Z

@metacpp what's the status of this PR - it's been a WIP for a while and has merge conflicts?

tombuildsstuff · 2018-07-12T14:51:47Z

@metacpp since we've not heard back from you here (and there's merge conflicts) - I'm going to close this PR for the moment; please feel free to re-open it if this is being actively worked on.

Thanks!

ghost · 2019-03-06T14:09:23Z

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!

metacpp added 2 commits April 12, 2018 23:17

Init work to support GitHub as deployment option.

30154a2

Integrate with Azure Go SDK to support GitHub:

6276b2f

1. Add update operation for source_control and external_scm_credential. 2. Refactor the code logic of expand and flatten functions.

metacpp added the service/app-service label Apr 14, 2018

metacpp added this to the 1.4.0 milestone Apr 14, 2018

metacpp added the enhancement label Apr 14, 2018

metacpp mentioned this pull request Apr 14, 2018

Attempting to add further SCM types to app_services #1103

Closed

tombuildsstuff added the waiting-response label Apr 14, 2018

tombuildsstuff modified the milestones: 1.4.0, Soon Apr 16, 2018

tombuildsstuff suggested changes Apr 16, 2018

View reviewed changes

metacpp requested a review from JunyiYi April 16, 2018 20:47

metacpp added 2 commits April 17, 2018 18:24

Fix nil pointer issue.

1827d71

Fix the missed package name for SiteSourceControlProperties.

ed9661f

metacpp self-assigned this Apr 18, 2018

Merge branch 'master' into github_scm

10444c6

tombuildsstuff mentioned this pull request Apr 27, 2018

Question about app_service #1174

Closed

metacpp added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label May 2, 2018

metacpp added 3 commits May 3, 2018 17:06

Update the schema to better support 3rd party deployment.

be17572

Merge commits from master and resolved conflicts.

105ad6b

renamed the personal access token and set it before update source con…

72d94a4

…trol.

metacpp mentioned this pull request Jun 10, 2018

SCM deployment support Azure Web App #1375

Closed

metacpp modified the milestones: Soon, 1.7.0 Jun 10, 2018

katbyte modified the milestones: 1.7.0, Soon Jun 12, 2018

tombuildsstuff closed this Jul 12, 2018

tombuildsstuff modified the milestones: Soon, Being Sorted Oct 25, 2018

tombuildsstuff deleted the github_scm branch October 27, 2018 20:00

ghost locked and limited conversation to collaborators Mar 6, 2019

ghost removed the waiting-response label Mar 6, 2019

Uh oh!

Conversation

metacpp commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metacpp commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tombuildsstuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacpp Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacpp Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tombuildsstuff Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacpp commented Apr 16, 2018

Uh oh!

metacpp commented May 2, 2018

Uh oh!

metacpp commented May 2, 2018

Uh oh!

tombuildsstuff commented Jun 19, 2018

Uh oh!

tombuildsstuff commented Jul 12, 2018

Uh oh!

ghost commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

metacpp commented Apr 14, 2018 •

edited

Loading

metacpp commented Apr 14, 2018 •

edited

Loading

metacpp Apr 16, 2018 •

edited

Loading

metacpp Apr 16, 2018 •

edited

Loading

tombuildsstuff Apr 17, 2018 •

edited

Loading