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

Service Fabric server certificate common names #3652

Merged
merged 12 commits into from
Jun 20, 2019

Conversation

steve-hawkins
Copy link
Contributor

certificate common names can be used in place of thumbprints as recommended for production clusters

makes certificate rotation process faster / safer as the manifest does not require updating and a rolling upgrade

more info on the Microsoft docs

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @steve-hawkins. These changes are looking good. I've got a few minor comments and one of the tests isn't passing.

--- FAIL: TestAccAzureRMServiceFabricCluster_readerAdminClientCertificateThumbprint (62.20s)
    testing.go:568: Step 0 error: Check failed: Check 5/12 error: azurerm_service_fabric_cluster.test: Attribute 'client_certificate_thumbprint.#' expected "2", got "0"

We should be able to get this merged in once these comments have been addressed

website/docs/r/service_fabric_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/service_fabric_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/service_fabric_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/service_fabric_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/service_fabric_cluster.html.markdown Outdated Show resolved Hide resolved
azurerm/resource_arm_service_fabric_cluster.go Outdated Show resolved Hide resolved
@steve-hawkins
Copy link
Contributor Author

@mbfrahry thanks for the feedback

out of interest, how come the failed test wasn't picked up in the CI build?

@ghost ghost removed the waiting-response label Jun 19, 2019
@ghost ghost added size/XL and removed size/L labels Jun 19, 2019
@mbfrahry
Copy link
Member

We've got a couple of different CI systems in place for providers. Travis lints, vets, runs unit tests, etc and then we have TeamCity which runs our acceptance tests. We've got TeamCity hooked up privately for now so we can tailor what gets run on specific branches and so it doesn't get abused. There are plans to get the results more public facing but no details as of yet to share out.

@steve-hawkins
Copy link
Contributor Author

@mbfrahry thanks for the info

let me know if there is anything else you wanted me to change as part of this PR

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

So close! Just some very minor test formatting fixes and a final test check and we're there.

azurerm/resource_arm_service_fabric_cluster_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_service_fabric_cluster_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_service_fabric_cluster_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_service_fabric_cluster_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_service_fabric_cluster_test.go Outdated Show resolved Hide resolved
@steve-hawkins
Copy link
Contributor Author

@mbfrahry thanks, vscode auto-format gets me every time on the Terraform with golang files

@mbfrahry mbfrahry merged commit d83b914 into hashicorp:master Jun 20, 2019
@mbfrahry
Copy link
Member

No worries @steve-hawkins. @katbyte wrote a pretty neat tool that formats hcl. It's still a WIP but it should help out here going forward. https://github.com/katbyte/terrafmt. Other than that, this looks good! Thanks so much for your time here.

@ghost
Copy link

ghost commented Jul 21, 2019

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 Jul 21, 2019
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.

3 participants