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

New Resource: azurerm_spring_cloud_service #4928

Merged
merged 8 commits into from
Apr 2, 2020

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented Nov 20, 2019

the first PR of issue #4891
this PR includes changes:

  1. spring cloud service
  2. spring cloud config server
  3. add relevant azure spring cloud go sdk to vender

config server is a property of spring cloud service, but currently the create api does not take care of config server part, it is a part of UPDATE operation by azure spring cloud team's design. So I split them and implement two different resource and datasource

@njuCZ
Copy link
Contributor Author

njuCZ commented Dec 2, 2019

Could anyone help review this PR?
This PR is about setting up Azure Spring Cloud Instance and spring cloud config server.
There is some following work about spring cloud application and deployment which rely on this PR

@njuCZ
Copy link
Contributor Author

njuCZ commented Dec 6, 2019

@katbyte Could you please have a review of this PR ?

@njuCZ njuCZ force-pushed the springcloud branch 2 times, most recently from f1b526c to 5dff6f9 Compare December 13, 2019 07:44
@njuCZ njuCZ closed this Dec 17, 2019
@njuCZ njuCZ reopened this Dec 17, 2019
@njuCZ njuCZ requested a review from mbfrahry January 19, 2020 02:48
@katbyte katbyte changed the title implement resource and data_source of Azure Spring Cloud New Resource: azurerm_spring_cloud Jan 20, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @njuCZ, I've given this a initial review

@njuCZ
Copy link
Contributor Author

njuCZ commented Jan 22, 2020

@katbyte thanks for your suggestion, I have modified the codes, Could you please have a look again?

@njuCZ
Copy link
Contributor Author

njuCZ commented Mar 5, 2020

@katbyte Could you have a review of the modified revision? I am sorry I don't know how to dismiss the Changes requested status

@tombuildsstuff tombuildsstuff modified the milestones: v2.3.0, v2.4.0 Mar 26, 2020
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @njuCZ
I've left some comments and change requests, mostly around documentation and a little refactor on the schema to reduce repetition.
There's also a question on API behaviour for the exclusivity of the auth type for repos.
Thanks
Ste

website/docs/r/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
website/docs/r/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
@njuCZ
Copy link
Contributor Author

njuCZ commented Mar 31, 2020

@jackofallops thanks for your review suggestions. I have refined the PR accordingly. Could you please have a look again ?

@ghost ghost removed the waiting-response label Mar 31, 2020
@njuCZ njuCZ requested a review from jackofallops March 31, 2020 08:26
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @njuCZ
Thanks for the changes, looking good. Just a few minor things left I think, mostly for clarity. There's one additional condition I think we should have in there, commented below about guarding against the user specifying both auth types for the config server and pattern repos.

Thanks!

Comment on lines 373 to 389
if httpBasicAuth := v["http_basic_auth"].([]interface{}); len(httpBasicAuth) > 0 {
v := httpBasicAuth[0].(map[string]interface{})
result.Username = utils.String(v["username"].(string))
result.Password = utils.String(v["password"].(string))
}
if sshAuth := v["ssh_auth"].([]interface{}); len(sshAuth) > 0 {
v := sshAuth[0].(map[string]interface{})
result.PrivateKey = utils.String(v["private_key"].(string))
result.StrictHostKeyChecking = utils.Bool(v["strict_host_key_checking_enabled"].(bool))

if hostKey := v["host_key"].(string); hostKey != "" {
result.HostKey = utils.String(hostKey)
}
if hostKeyAlgorithm := v["host_key_algorithm"].(string); hostKeyAlgorithm != "" {
result.HostKeyAlgorithm = utils.String(hostKeyAlgorithm)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

as with the top level repo, we should test against these both being set and error out to the user to prevent both being attempted to be sent to the API since only one will be accepted (based on your comments from the service team that this is conditional based on the protocol used in the URI).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackofallops I have added check to prevent setting both the fields, please have a look. Thanks for your suggestion

@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 1, 2020

@jackofallops thanks for the suggestion, Could you please have a look again?

@njuCZ njuCZ requested a review from jackofallops April 2, 2020 01:44
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @njuCZ
Thanks again for the changes - looking really close now. Just one schema item (sorry I missed this on previous review) and a slight rewording on a bit of the docs and I believe this will be ready.

Thanks

@@ -0,0 +1,132 @@
subcategory: "App Platform"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subcategory: "App Platform"
---
subcategory: "App Platform"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackofallops just curious about this rule, because in my previous comments, Tom told me to remove the first line. #4928 (comment)
so in my future PRs, should I add the first line --- ?

website/docs/r/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
website/docs/d/spring_cloud_service.html.markdown Outdated Show resolved Hide resolved
@jackofallops
Copy link
Member

Hi @njuCZ - Hope you don't mind, I pushed the final suggestions so we can close and merge this asap. Many thanks again for this work.

@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 2, 2020

many thanks!

@katbyte katbyte merged commit 136a911 into hashicorp:master Apr 2, 2020
katbyte added a commit that referenced this pull request Apr 2, 2020
@ghost
Copy link

ghost commented Apr 2, 2020

This has been released in version 2.4.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.4.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented May 3, 2020

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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants