Add certificate support for Azure Batch #3097

stuartleeks · 2019-03-21T10:23:36Z

tombuildsstuff

Thanks for this PR :)

I've taken a look through and left some comments inline (most of which are minor/for consistency with other resources) but this is looking pretty good - if we can fix those comments up (and the tests pass) then we should be able to get this merged 👍

Thanks!

azurerm/data_source_batch_certificate.go

azurerm/data_source_batch_pool_test.go

website/docs/r/batch_certificate.html.markdown

website/docs/r/batch_pool.html.markdown

stuartleeks · 2019-03-28T11:19:41Z

@tombuildsstuff , thanks for the review!

I've pushed a few commits that address most of the feedback (have marked those conversations as resolved) and also rebased on master to keep the changes fresh. Waiting on the build to be queued currently.

There are still a couple of discussion points that I'd value your feedback on.

stuartleeks · 2019-03-29T09:10:34Z

@tombuildsstuff - just checked back and see that the build/tests passed ok in CI build :-)

Let me know your thoughts on the changes/remaining questions and whether you are happy with the PR (or have further changes you would like)

tombuildsstuff

hey @stuartleeks

Thanks for pushing those changes; I've taken a look through and left some additional (minor) comments in-line but this mostly LGTM - if we can fix those comments up we should be able to run the tests and get this merged 👍

Thanks!

azurerm/data_source_batch_pool.go

azurerm/helpers/azure/batch_pool.go

website/docs/d/batch_certificate.html.markdown

website/docs/r/batch_certificate.html.markdown

tombuildsstuff · 2019-03-29T10:09:27Z

@stuartleeks I've also just noticed the file azurerm/testdata/packages-microsoft-prod.deb in this PR which I believe wants removing?

Improves plan diff and output (aligns with ARM input/output)

stuartleeks · 2019-04-01T08:12:43Z

Latest changes made (with the exception of one in docs where I wasn't sure which part you were referring to).
Rebased onto current master

stuartleeks · 2019-04-04T11:34:31Z

@katbyte - I just realised that some of the comments in your review were on comments in @tombuildsstuff 's review but they weren't shown by default. Just responded to them inline now. Sorry for the confusion!

tombuildsstuff

hey @stuartleeks

Thanks for pushing those commits - this is now looking good, if we can fix up the minor remaining issues (and raise a bug in the Azure Swagger repo) this otherwise LGTM 👍

Thanks!

azurerm/data_source_batch_certificate.go

azurerm/resource_arm_batch_certificate.go

tombuildsstuff · 2019-04-06T08:29:18Z

azurerm/resource_arm_batch_pool.go

+							Type:             schema.TypeString,
+							Required:         true,
+							ValidateFunc:     validate.NoEmptyStrings,
+							DiffSuppressFunc: suppress.CaseDifference,


we should raise a bug about this API behaviour on the Rest API Specs Repository since the Resource ID should be returned in a consistent manner (whilst the ARM spec technically defines a Resource ID as case-insensitive; in practice both other ARM API's and downstream tooling don't handle these being case-insensitive - such that the casing for an ID should be the same within a Resource Provider API).

could we also add a TODO here with a link to the upstream API issue, when that's created?

stuartleeks · 2019-04-08T12:38:58Z

@tombuildsstuff - I've made the changes you requested (including adding a comment referencing the issue created in the azure swagger repo: Azure/azure-rest-api-specs#5574)

Let me know if there's anything else needed to get this merged :-)

tombuildsstuff

hey @stuartleeks

Thanks for pushing those changes - this now LGTM 👍

Thanks!

dismissing since changes have been pushed

katbyte

Hi @stuartleeks,

After running the tests I believe you may have a bug in the provider:

Test ended in panic.

------- Stdout: -------
=== RUN   TestAccAzureRMBatchCertificatePfx
=== PAUSE TestAccAzureRMBatchCertificatePfx
=== CONT  TestAccAzureRMBatchCertificatePfx

------- Stderr: -------
panic: Invalid address to set: []string{"public_data"}

goroutine 257 [running]:
github.com/hashicorp/terraform/helper/schema.(*ResourceData).Set(0xc0004ac070, 0x2bf0c5b, 0xb, 0x2806880, 0xc00068c540, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/resource_data.go:191 +0x334
github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmBatchCertificateRead(0xc0004ac070, 0x2bb1100, 0xc000627000, 0xc0009c65a0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_batch_certificate.go:163 +0x857
github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmBatchCertificateCreate(0xc0004ac070, 0x2bb1100, 0xc000627000, 0xc0004ac070, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_batch_certificate.go:132 +0xe42
github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc0004b0620, 0xc0005dd9a0, 0xc000c1b420, 0x2bb1100, 0xc000627000, 0xc0006ecb01, 0x3e, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:225 +0x351
github.com/hashicorp/terraform/helper/schema.(*Provider).Apply(0xc00036fa40, 0xc0005dd310, 0xc0005dd9a0, 0xc000c1b420, 0x1, 0x4813d3, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:283 +0x9c
github.com/hashicorp/terraform/terraform.(*EvalApply).Eval(0xc000c30980, 0x317ce00, 0xc00073cdd0, 0x2, 0x2, 0x2be6b2c, 0x4)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/terraform/eval_apply.go:57 +0x226
github.com/hashicorp/terraform/terraform.EvalRaw(0x3138d60, 0xc000c30980, 0x317ce00, 0xc00073cdd0, 0x0, 0x0, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/terraform/eval.go:53 +0x156
github.com/hashicorp/terraform/terraform.(*EvalSequence).Eval(0xc000c1ace0, 0x317ce00, 0xc00073cdd0, 0x2, 0x2, 0x2be6b2c, 0x4)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/terraform/eval_sequence.go:14 +0x9c
github.com/hashicorp/terraform/terraform.EvalRaw(0x31392a0, 0xc000c1ace0, 0x317ce00, 0xc00073cdd0, 0x288e9a0, 0x4b504a2, 0x2806880, 0xc0006fd730)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/terraform/eval.go:53 +0x156
github.com/hashicorp/terraform/terraform.Eval(0x31392a0, 0xc000c1ace0, 0x317ce00, 0xc00073cdd0, 0xc000c1ace0, 0x31392a0, 0xc000c1ace0, 0x2e55dc0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/terraform/eval.go:34 +0x4d
github.com/hashicorp/terraform/terraform.(*Graph).walk.func1(0x2b3d200, 0xc00000fe20, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/terraform/graph.go:126 +0xc45
github.com/hashicorp/terraform/dag.(*Walker).walkVertex(0xc0008a01c0, 0x2b3d200, 0xc00000fe20, 0xc0009be940)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/dag/walk.go:387 +0x367
created by github.com/hashicorp/terraform/dag.(*Walker).Update
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/dag/walk.go:310 +0x986

stuartleeks · 2019-04-09T20:48:30Z

@katbyte - I'll take a look back over the code around that area. Did you make any changes to the tests? (I didn't see that error on my machine or in the Travis build)

tombuildsstuff · 2019-04-10T06:49:01Z

@stuartleeks this panic's caused by an Environment Variable (TF_SCHEMA_PANIC_ON_ERROR=1) set in the Makefile and on our CI server which causes a panic if you attempt to set a field which doesn't exist in the schema (or set the wrong type); depending on how you're running the tests it's possible that may not be set.

Taking a look into this it appears the public_data field is missing from the azurerm_batch_certificate resource (but is present in the data source), which is why this is failing - as such I'm going to push a commit to add this field in and re-run the tests, I hope you don't mind!

Thanks!

dismissing since changes have been pushed

stuartleeks · 2019-04-10T15:02:37Z

Great - thanks @tombuildsstuff :-)

stuartleeks · 2019-04-12T08:26:47Z

@tombuildsstuff / @katbyte are there any other changes that you want on this before it is merged? Do you want it rebased to the latest master?

katbyte · 2019-04-12T18:23:44Z

nope! we just had some issues moving to a new subscription and increasing batch account quota so we could easily test.

Just ran them now and LGTM @stuartleeks 🙂

ghost · 2019-04-17T10:40:20Z

This has been released in version 1.25.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 = "~> 1.25.0"
}
# ... other configuration ...

ghost · 2019-05-13T13:50:30Z

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!

ghost added size/XXL documentation labels Mar 21, 2019

tombuildsstuff added service/batch new-resource new-data-source labels Mar 21, 2019

tombuildsstuff suggested changes Mar 21, 2019

View reviewed changes

tombuildsstuff added the waiting-response label Mar 21, 2019

stuartleeks force-pushed the batch-certs branch from 35cedb7 to 6c7704f Compare March 28, 2019 10:56

ghost removed the waiting-response label Mar 28, 2019

tombuildsstuff suggested changes Mar 29, 2019

View reviewed changes

tombuildsstuff added the waiting-response label Mar 29, 2019

stuartleeks force-pushed the batch-certs branch from 6c7704f to 4fa140d Compare March 29, 2019 16:46

stuartleeks and others added 11 commits April 1, 2019 10:03

Initial code to create Batch Certificates

78cd480

Switch to base 64 encoded resource_arm_batch_certificate

688b5e0

Improves plan diff and output (aligns with ARM input/output)

Set default for store_location

7a41371

Add batch cert data source and more tests

c840117

Add default value for certificate type

f46f6be

Add docs

52ab3d7

Add batch_certificate sidebar in docs

3c7a1fa

Accept PR suggestions from @tombuildsstuff

fed4922

Tweak based on review comments

9476478

Make Certificate Name a computed property

391c988

Further PR feedback

804afda

stuartleeks force-pushed the batch-certs branch from 4fa140d to 804afda Compare April 1, 2019 08:04

ghost removed the waiting-response label Apr 1, 2019

Fix comment error

943caf3

tombuildsstuff approved these changes Apr 6, 2019

View reviewed changes

tombuildsstuff added this to the v1.25.0 milestone Apr 6, 2019

tombuildsstuff added the waiting-response label Apr 6, 2019

stuartleeks mentioned this pull request Apr 8, 2019

Inconsistent casing in Batch Certificate IDs Azure/azure-rest-api-specs#5574

Closed

stuartleeks force-pushed the batch-certs branch 2 times, most recently from 7efa33c to 3179d24 Compare April 8, 2019 12:24

PR review comments

e9c970f

stuartleeks force-pushed the batch-certs branch from 3179d24 to e9c970f Compare April 8, 2019 12:28

ghost removed the waiting-response label Apr 8, 2019

tombuildsstuff approved these changes Apr 9, 2019

View reviewed changes

katbyte previously requested changes Apr 9, 2019

View reviewed changes

katbyte added the waiting-response label Apr 9, 2019

ghost removed the waiting-response label Apr 9, 2019

r/batch_certificate: exposing the public_data field

e8c27b3

r/batch_certificate: documenting the public_data field

3cea650

katbyte merged commit 387fa9b into hashicorp:master Apr 12, 2019

katbyte added a commit that referenced this pull request Apr 12, 2019

Update CHANGELOG.md to include #3097

4aeb146

stuartleeks mentioned this pull request Apr 15, 2019

Add resource_file to batch pool start_task #3192

Merged

ghost locked and limited conversation to collaborators May 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add certificate support for Azure Batch #3097

Add certificate support for Azure Batch #3097

stuartleeks commented Mar 21, 2019

tombuildsstuff left a comment

stuartleeks commented Mar 28, 2019

stuartleeks commented Mar 29, 2019

tombuildsstuff left a comment

tombuildsstuff commented Mar 29, 2019

stuartleeks commented Apr 1, 2019

stuartleeks commented Apr 4, 2019

tombuildsstuff left a comment

tombuildsstuff Apr 6, 2019

stuartleeks commented Apr 8, 2019

tombuildsstuff left a comment

katbyte left a comment

stuartleeks commented Apr 9, 2019

tombuildsstuff commented Apr 10, 2019

stuartleeks commented Apr 10, 2019

stuartleeks commented Apr 12, 2019

katbyte commented Apr 12, 2019

ghost commented Apr 17, 2019

ghost commented May 13, 2019

Add certificate support for Azure Batch #3097

Add certificate support for Azure Batch #3097

Conversation

stuartleeks commented Mar 21, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

stuartleeks commented Mar 28, 2019

stuartleeks commented Mar 29, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 29, 2019

stuartleeks commented Apr 1, 2019

stuartleeks commented Apr 4, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Apr 6, 2019

Choose a reason for hiding this comment

stuartleeks commented Apr 8, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

stuartleeks commented Apr 9, 2019

tombuildsstuff commented Apr 10, 2019

stuartleeks commented Apr 10, 2019

stuartleeks commented Apr 12, 2019

katbyte commented Apr 12, 2019

ghost commented Apr 17, 2019

ghost commented May 13, 2019