Support for Azure Batch - Pool #2461

jcorioland · 2018-12-06T08:08:10Z

This PR brings support for Azure Batch Pool.

Fixes #2427 and #2458
Depends on #2428

Azure Batch Pool:

tombuildsstuff · 2018-12-10T14:55:11Z

@jcorioland thanks for splitting this out :)

Given this is dependent on #2427 - just to let you know I'm going to take a look through the commit specific to this PR, rather than attempting to re-review the bits present in the other PR (to hopefully avoids any conflicts)

…provider-azurerm into feat/batch-pool

tombuildsstuff · 2018-12-14T10:55:51Z

@jcorioland I've merged master into this branch so those changes don't appear in the diff now that the other PR's been merged, I hope you don't mind

jcorioland · 2018-12-14T11:01:28Z

@tombuildsstuff nope, this is perfect, thank you.
Was looking on the issue #2458 - not sure it require a new resource and being implemented in another PR as the start task is actually part of the Batch PoolProperties in the Azure SDK for Go.
So I just need to add required properties in the schema of resource/data source and implement the start task in the Create/Update/Read functions.

What do you think about? Better to do it in this PR or a new one?
Thanks

…vider-azurerm into feat/batch-pool

katbyte

@jcorioland,

Thank you for the resource! I've left some comments inline and its off to a good start. My main concern is that i think the autoscale settings should be moved into sub blocks to match the API, as well as allow for some simpler logic.

With regards to the Start Task i think it should be a sub block in this resource. It could be part of this PR or another 🙂

katbyte · 2018-12-17T23:00:35Z

azurerm/data_source_batch_pool.go

+			},
+			"storage_image_reference": {
+				Type:     schema.TypeSet,
+				Optional: true,


I believe we can remove this as it's a datasource?

Suggested change

Optional: true,

katbyte · 2018-12-17T23:00:45Z

azurerm/data_source_batch_pool.go

+				Type:     schema.TypeSet,
+				Optional: true,
+				Computed: true,
+				ForceNew: true,


I believe we can remove this as it's a datasource?

Suggested change

ForceNew: true,

katbyte · 2018-12-17T23:00:52Z

azurerm/data_source_batch_pool.go

+				Optional: true,
+				Computed: true,
+				ForceNew: true,
+				MaxItems: 1,


I believe we can remove this as it's a datasource?

Suggested change

MaxItems: 1,

katbyte · 2018-12-17T23:01:49Z

azurerm/data_source_batch_pool.go

+					Schema: map[string]*schema.Schema{
+						"id": {
+							Type:     schema.TypeString,
+							Optional: true,


Should the ForceNew be removed and optional changed to Computed?

Yep, it should. Done.

katbyte · 2018-12-17T23:01:54Z

azurerm/data_source_batch_pool.go

+
+						"offer": {
+							Type:     schema.TypeString,
+							Optional: true,


Should the ForceNew be removed and optional changed to Computed?

Yep it should. Done.

katbyte · 2018-12-17T23:15:57Z

azurerm/resource_arm_batch_pool_test.go

+	vm_size				   = "Standard_A1"
+	scale_mode			   = "Fixed"
+	target_dedicated_nodes = 2
+	node_agent_sku_id	= "batch.node.ubuntu 16.04"


Could we fix the alignment here?

katbyte · 2018-12-17T23:16:19Z

azurerm/resource_arm_batch_pool_test.go

+	scale_mode			   		  = "Auto"
+	autoscale_evaluation_interval = "PT15M"
+	autoscale_formula			  = <<EOF
+	startingNumberOfVMs = 1;


Could we fix the assignment alignment here?

katbyte · 2018-12-17T23:17:13Z

examples/batch/main.tf

+  resource_group_name    = "${azurerm_resource_group.rg.name}"
+  account_name 		       = "${azurerm_batch_account.batch.name}"
+  display_name		       = "Fixed Scale Pool"
+  vm_size				         = "Standard_A1"


Could we run terraform ft on this file to fix the alignment? you can just point it at the folder and it'll find and fmt all terraform files 🙂

katbyte · 2018-12-17T23:17:42Z

website/docs/r/batch_pool.html.markdown

+}
+
+resource "azurerm_batch_pool" "test" {
+	name                   		    = "testaccpool"


Could we fix the alignment here?

katbyte · 2018-12-17T23:18:32Z

website/docs/r/batch_pool.html.markdown

+
+* `autoscale_formula` - The formula used to autoscale the pool when using scale mode `Auto`.
+
+* `storage_image_reference` - The reference of the storage image used by the nodes in the Batch pool.


Could we add an import section here?

jcorioland · 2018-12-18T15:16:47Z

@katbyte - Thanks for the review. I've done some refactoring and most of the changes you've asked for are done.
I will implement start task part of this PR, it does not make sense to create another one, indeed.

tombuildsstuff

hey @jcorioland

Thanks for pushing those changes - I've taken a look though and left some comments inline, in general this is off to a good start but I've got some questions/thoughts around the design of the schema (such as making things Lists rather than Sets)

Thanks!

tombuildsstuff · 2018-12-20T11:10:57Z

azurerm/data_source_batch_pool.go

+				Required:     true,
+				ValidateFunc: azure.ValidateAzureRMBatchPoolName,
+			},
+			"resource_group_name": resourceGroupNameDiffSuppressSchema(),


unfortunately whilst this is case insensitive in some Azure API's - in others (where these ID's can be use) they're not; as such we treat Resource Group Name's as case sensitive - and we need to update this to be case-sensitive:

Suggested change

"resource_group_name": resourceGroupNameDiffSuppressSchema(),

"resource_group_name": resourceGroupNameForDataSourceSchema(),

Ok, got it. Thanks for the explanation.

tombuildsstuff · 2018-12-20T11:13:23Z

azurerm/data_source_batch_pool.go

+			},
+			"fixed_scale":             azure.SchemaBatchPoolFixedScaleForDataSource(),
+			"auto_scale":              azure.SchemaBatchPoolAutoScaleForDataSource(),
+			"storage_image_reference": azure.SchemaBatchPoolImageReferenceForDataSource(),


since these are only used in a single place (and unlikely to be used outside of this resource) - we can define these in-line rather than splitting these out

tombuildsstuff · 2018-12-20T11:13:27Z

azurerm/data_source_batch_pool.go

+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"start_task": azure.SchemaBatchPoolStartTaskForDataSource(),


(same here)

tombuildsstuff · 2018-12-20T11:15:48Z

azurerm/data_source_batch_pool.go

+	d.Set("name", name)
+	d.Set("account_name", accountName)
+	d.Set("resource_group_name", resourceGroup)
+	d.Set("vm_size", resp.VMSize)


since these properties are present at properties in the response, can we access them via:

if props := resp.PoolProperties; props != nil { d.Set("vm_size", props.VMSize) // ... }

Yes. I am curious about the exact reason behind this?

The Azure API returns the representation of a resource at the last time it was modified; meaning if it’s not been modified in a while (for example if it was made during a Preview API) the structure returned from the API may not necessarily match the result we expect. As such we ensure we access these fields in the Properties block, since that’s where we expect them

tombuildsstuff · 2018-12-20T11:17:27Z

azurerm/data_source_batch_pool.go

+	d.Set("resource_group_name", resourceGroup)
+	d.Set("vm_size", resp.VMSize)
+
+	if resp.ScaleSettings != nil {


(same here) we should be able to make this:

if props := ...; props != nil { if settings := props.ScaleSettings; settings != nil { if err := d.Set("auto_scale", flattenBatchPoolAutoScaleSettings(settings.AutoScale)); err != nil { return fmt.Errorf("Error flattening `auto_scale`: %+v", err) } if err := d.Set("fixed_scale", flattenBatchPoolFixedScaleSettings(settings.FixedScale)); err != nil { return fmt.Errorf("Error flattening `fixed_scale `: %+v", err) } } }

in this updated snippet we also handle settings.AutoScale and settings.FixedScale being nil within the flatten function and return an empty list if that's the case

tombuildsstuff · 2018-12-20T12:29:54Z

azurerm/resource_arm_batch_pool.go

+	d.Set("resource_group_name", resourceGroup)
+	d.Set("vm_size", resp.VMSize)
+
+	if resp.ScaleSettings != nil {


(as above) can we access these within the properties block e.g.

if props := resp.PoolProperties; props != nil { // ... }

tombuildsstuff · 2018-12-20T12:55:31Z

azurerm/resource_arm_batch_pool.go

+			TargetLowPriorityNodes: &targetLowPriorityNodes,
+		}
+	} else {
+		return nil, fmt.Errorf("Error: scale mode should be either AutoScale of FixedScale")


this is covered above - so we should be able to remove this else block here?

tombuildsstuff · 2018-12-20T12:55:55Z

examples/batch/main.tf

-  # client_id       = "..." 
-  # client_secret   = "..."
-  # tenant_id       = "..."
+  # if you're using a Service Principal (shared account) then either set the environment variables, or fill these in:   # subscription_id = "..."  # client_id       = "..."   # client_secret   = "..."  # tenant_id       = "..."


can we put this back on multiple lines?

for the record, terraform fmt inline the comments :-)

tombuildsstuff · 2018-12-20T12:56:38Z

examples/batch/main.tf

+      pendingTaskSamples = pendingTaskSamplePercent < 70 ? startingNumberOfVMs : avg($PendingTasks.GetSample(180 * TimeInterval_Second));
+      $TargetDedicatedNodes=min(maxNumberofVMs, pendingTaskSamples);
+      EOF


for this to be detected correctly the EOF needs to be at the start of the line here:

Suggested change

EOF

EOF

tombuildsstuff · 2018-12-20T12:57:43Z

website/docs/r/batch_pool.html.markdown

+      pendingTaskSamplePercent = $PendingTasks.GetSamplePercent(180 * TimeInterval_Second);
+      pendingTaskSamples = pendingTaskSamplePercent < 70 ? startingNumberOfVMs : avg($PendingTasks.GetSample(180 *   TimeInterval_Second));
+      $TargetDedicatedNodes=min(maxNumberofVMs, pendingTaskSamples);
+      EOF


(as above) this needs to be at the start of the line to take effect:

Suggested change

EOF

EOF

jcorioland · 2018-12-20T19:28:45Z

@tombuildsstuff @katbyte - I've done updates after Tom's review. I have issues with the acceptance tests now. They all end with the following error:

--- FAIL: TestAccAzureRMBatchPool_basic (699.32s)
    testing.go:538: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        DESTROY/CREATE: azurerm_batch_pool.test
          account_name:                            "testaccbatchc3tm" => "testaccbatchc3tm"
          fixed_scale.#:                           "1" => "1"
          fixed_scale.0.resize_timeout:            "PT15M" => "PT15M"
          fixed_scale.0.target_dedicated_nodes:    "1" => "1"
          fixed_scale.0.target_low_priority_nodes: "0" => "0"
          name:                                    "testaccpoolc3tm" => "testaccpoolc3tm"
          node_agent_sku_id:                       "batch.node.ubuntu 16.04" => "batch.node.ubuntu 16.04"
          resource_group_name:                     "testaccbatch8903972532148945948" => "testaccbatch8903972532148945948"
          stop_pending_resize_operation:           "false" => "false"
          storage_image_reference.#:               "1" => "1"
          storage_image_reference.0.offer:         "UbuntuServer" => "UbuntuServer"
          storage_image_reference.0.publisher:     "Canonical" => "Canonical"
          storage_image_reference.0.sku:           "16.04.0-LTS" => "16.04.0-LTS"
          storage_image_reference.0.version:       "latest" => "latest"
          vm_size:                                 "STANDARD_A1" => "Standard_A1" (forces new resource)
        
        STATE:
        
        azurerm_batch_account.test:
          ID = /subscriptions/SUB_ID/resourceGroups/testaccbatch8903972532148945948/providers/Microsoft.Batch/batchAccounts/testaccbatchc3tm
          provider = provider.azurerm
          location = northeurope
          name = testaccbatchc3tm
          pool_allocation_mode = BatchService
          resource_group_name = testaccbatch8903972532148945948
          tags.% = 0
       
Dependencies:
            azurerm_resource_group.test
        azurerm_batch_pool.test:
          ID = /subscriptions/SUB_ID/resourceGroups/testaccbatch8903972532148945948/providers/Microsoft.Batch/batchAccounts/testaccbatchc3tm/pools/testaccpoolc3tm
          provider = provider.azurerm
          account_name = testaccbatchc3tm
          auto_scale.# = 0
          fixed_scale.# = 1
          fixed_scale.0.resize_timeout = PT15M
          fixed_scale.0.target_dedicated_nodes = 1
          fixed_scale.0.target_low_priority_nodes = 0
          name = testaccpoolc3tm
          node_agent_sku_id = batch.node.ubuntu 16.04
          resource_group_name = testaccbatch8903972532148945948
          stop_pending_resize_operation = false
          storage_image_reference.# = 1
          storage_image_reference.0.id = 
          storage_image_reference.0.offer = UbuntuServer
          storage_image_reference.0.publisher = Canonical
          storage_image_reference.0.sku = 16.04.0-LTS
          storage_image_reference.0.version = latest
          vm_size = STANDARD_A1
        
          Dependencies:
              azurerm_batch_account.test
            azurerm_resource_group.test
        azurerm_resource_group.test:
          ID = /subscriptions/SUB_ID/resourceGroups/testaccbatch8903972532148945948
          provider = provider.azurerm
          location = northeurope
          name = testaccbatch8903972532148945948
          tags.% = 0
FAIL

I am not sure of what happens exactly, if you have any clue that would be really helpful. Thanks!

katbyte · 2018-12-21T02:21:27Z

@jcorioland,

It appears to be a casing issue with vm_size being returned in all caps from the API? causing it to want to re-create the resource. Looks like that property is going to have to be case insensitive (and a bug should probably be opened with Azure as that seems like an error)

jcorioland · 2018-12-21T09:48:28Z

@katbyte thanks a lot, making the vmSize case insensitive solved the issue. All tests pass now! As you've suggested, I've opened an issue on the rest api specs repo to clarify this.
I think that I've fixed all the issues that you and @tombuildsstuff raised out during your reviews.
LMK if I need to do other changes.
Thanks

tombuildsstuff · 2018-12-23T15:15:59Z

Data Source test passes:

katbyte

hi @jcorioland,

I've tried running the tests again and gotten this error:

------- Stdout: -------
=== RUN   TestAccAzureRMBatchPool_autoScale_complete
=== PAUSE TestAccAzureRMBatchPool_autoScale_complete
=== CONT  TestAccAzureRMBatchPool_autoScale_complete
--- FAIL: TestAccAzureRMBatchPool_autoScale_complete (853.77s)
	testing.go:538: Step 0 error: After applying this step, the plan was not empty:
		
		DIFF:
		
		UPDATE: azurerm_batch_pool.test
		  auto_scale.0.formula: "startingNumberOfVMs = 1;\n      maxNumberofVMs = 25;\n      pendingTaskSamplePercent = $PendingTasks.GetSamplePercent(180 * TimeInterval_Second);\n      pendingTaskSamples = pendingTaskSamplePercent < 70 ? startingNumberOfVMs : avg($PendingTasks.GetSample(180 * TimeInterval_Second));\n      $TargetDedicatedNodes=min(maxNumberofVMs, pendingTaskSamples);" => "      startingNumberOfVMs = 1;\n      maxNumberofVMs = 25;\n      pendingTaskSamplePercent = $PendingTasks.GetSamplePercent(180 * TimeInterval_Second);\n      pendingTaskSamples = pendingTaskSamplePercent < 70 ? startingNumberOfVMs : avg($PendingTasks.GetSample(180 * TimeInterval_Second));\n      $TargetDedicatedNodes=min(maxNumberofVMs, pendingTaskSamples);\n"

FAIL

The difference:

      "startingNumberOfVMs = 1;\n      maxNumberofVMs = 25;\n      pendingTaskSamplePercent = $PendingTasks.GetSamplePercent(180 * TimeInterval_Second);\n      pendingTaskSamples = pendingTaskSamplePercent < 70 ? startingNumberOfVMs : avg($PendingTasks.GetSample(180 * TimeInterval_Second));\n      $TargetDedicatedNodes=min(maxNumberofVMs, pendingTaskSamples);"

"      startingNumberOfVMs = 1;\n      maxNumberofVMs = 25;\n      pendingTaskSamplePercent = $PendingTasks.GetSamplePercent(180 * TimeInterval_Second);\n      pendingTaskSamples = pendingTaskSamplePercent < 70 ? startingNumberOfVMs : avg($PendingTasks.GetSample(180 * TimeInterval_Second));\n      $TargetDedicatedNodes=min(maxNumberofVMs, pendingTaskSamples);\n"

It seems the API is trimming the value and adding a newline to the end, might best to add a suppress diff function to this property. Also i noticed a bunch of other properties don't have any validation, it would be great to add some in.

katbyte · 2018-12-24T19:34:00Z

azurerm/resource_arm_batch_pool.go

+							Type:     schema.TypeInt,
+							Optional: true,
+							Default:  1,
+						},


Could we get some validation here?

katbyte · 2018-12-24T19:34:18Z

azurerm/resource_arm_batch_pool.go

+						"target_low_priority_nodes": {
+							Type:     schema.TypeInt,
+							Optional: true,
+							Default:  0,


Could we get some validation here?

katbyte · 2018-12-24T19:35:52Z

azurerm/resource_arm_batch_pool.go

+						"id": {
+							Type:     schema.TypeString,
+							Optional: true,
+						},


Could we validate the ID: azure.ValidateResourceID

katbyte · 2018-12-24T19:36:30Z

azurerm/resource_arm_batch_pool.go

+							Type:     schema.TypeString,
+							Required: true,
+							ForceNew: true,
+						},


Could we get at least a validate.NoEmptyStrings here?

katbyte · 2018-12-24T19:36:34Z

azurerm/resource_arm_batch_pool.go

+							Type:     schema.TypeString,
+							Required: true,
+							ForceNew: true,
+						},


Could we get at least a validate.NoEmptyStrings here?

katbyte · 2018-12-24T19:36:46Z

azurerm/resource_arm_batch_pool.go

+							Type:     schema.TypeString,
+							Required: true,
+							ForceNew: true,
+						},


Could we get at least a validate.NoEmptyStrings here?

katbyte · 2018-12-24T19:36:48Z

azurerm/resource_arm_batch_pool.go

+							Required:         true,
+							ForceNew:         true,
+							DiffSuppressFunc: suppress.CaseDifference,
+						},


Could we get at least a validate.NoEmptyStrings here?

katbyte · 2018-12-24T19:37:14Z

azurerm/resource_arm_batch_pool.go

+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,
+			},


Could we get at least a validate.NoEmptyStrings here?

katbyte · 2018-12-24T19:37:41Z

azurerm/resource_arm_batch_pool.go

+						"command_line": {
+							Type:     schema.TypeString,
+							Required: true,
+						},


Could we get at least a validate.NoEmptyStrings here?

katbyte · 2018-12-24T19:38:06Z

azurerm/resource_arm_batch_pool.go

+
+						"user_identity": {
+							Type:     schema.TypeList,
+							Required: true,


Should this we required when all the sub properties are optional?

I was not sure for this one: user identity is mandatory, but you should specify either a user name or an auto identity. This is why I put the user_identity block mandatory but the two properties inside optional. What the best way to handle that? Adding some extra validation to the user_identity block to check that at least one type of identity has been specified?

If one of those values is required i would add an additional check & document it in the create/update functions for now.

jcorioland · 2019-01-07T13:36:12Z

Hi @katbyte @tombuildsstuff - Happy New Year!
Thanks for your reviews.

I've added some validations, except for the user_identity, not sure on the best way to handle that case? (cf. my comment).
I don't have any issue with the acceptance test on autoscale, but I've added a suppress func to trim the string, as you've suggested.

Thank you

katbyte · 2019-01-08T04:54:12Z

Happy new year @jcorioland! Thanks for the commits.

If one of those values is required i would add a check for it into the create/update functions. A CutomizeDiff would be better to check during the plan but it doesn't work with interpolated values at the moment.

jcorioland · 2019-01-08T09:59:54Z

@katbyte thanks! I've just added validation for user identity. Should be good now.

katbyte

Thanks @jcorioland! LGTM now 🙂

ghost · 2019-03-05T14:11:02Z

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!

wip: support for Azure Batch pool

8e754ee

ghost added size/XXL dependencies labels Dec 6, 2018

jcorioland mentioned this pull request Dec 6, 2018

Support for Azure Batch - Account #2428

Merged

5 tasks

jcorioland added 3 commits December 12, 2018 13:31

Merge branch 'feat/azurerm_batch' of github.com:jcorioland/terraform-…

3f3d888

…provider-azurerm into feat/batch-pool

wip: docs for batch pool

65805a8

wip: batch pool creation + acc tests

5bdaf1f

ghost added the documentation label Dec 12, 2018

jcorioland added 3 commits December 13, 2018 16:52

Batch pool update + delete

369bec9

Merge branch 'feat/azurerm_batch' of github.com:jcorioland/terraform-…

7517253

…provider-azurerm into feat/batch-pool

update Batch example to add pool resources

90e6efb

jcorioland changed the title ~~[WIP] Support for Azure Batch - Pool~~ Support for Azure Batch - Pool Dec 13, 2018

jcorioland added 3 commits December 13, 2018 17:24

fix indent in Batch sample

6e22e0c

fix linters issue

38af409

Merge branch 'feat/azurerm_batch' of github.com:jcorioland/terraform-…

05a535f

…provider-azurerm into feat/batch-pool

jcorioland mentioned this pull request Dec 14, 2018

Feature Request: Batch Pool Start Task #2458

Closed

Merge branch 'master' into feat/batch-pool

e5d2507

Merge branch 'feat/batch-pool' of github.com:jcorioland/terraform-pro…

9a3da92

…vider-azurerm into feat/batch-pool

tombuildsstuff added the service/batch label Dec 14, 2018

katbyte requested changes Dec 17, 2018

View reviewed changes

katbyte added the waiting-response label Dec 17, 2018

jcorioland changed the title ~~Support for Azure Batch - Pool~~ [WIP] Support for Azure Batch - Pool Dec 18, 2018

jcorioland added 3 commits December 18, 2018 15:36

refactoring on Batch pool

276f3ca

update samples

59b511f

fix linters issues

cacd2e3

ghost removed the waiting-response label Dec 18, 2018

tombuildsstuff requested changes Dec 20, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Dec 20, 2018

refactoring after review

88ff167

ghost removed the waiting-response label Dec 20, 2018

few fixes

05b0893

jcorioland mentioned this pull request Dec 21, 2018

[Batch Pool] REST API returns vmSize property in upper case ? Azure/azure-rest-api-specs#4956

Open

ignore case for vmSize + improve acc tests

fc0deca

jcorioland changed the title ~~[WIP] Support for Azure Batch - Pool~~ Support for Azure Batch - Pool Dec 21, 2018

update examples+docs for Batch pool

9c398cf

katbyte requested changes Dec 24, 2018

View reviewed changes

katbyte added the waiting-response label Dec 24, 2018

add validation to batch pool resource

54ac930

ghost removed the waiting-response label Jan 7, 2019

katbyte added the waiting-response label Jan 8, 2019

add validation for user identity

d3525ad

ghost removed the waiting-response label Jan 8, 2019

katbyte approved these changes Jan 8, 2019

View reviewed changes

katbyte added this to the 1.21.0 milestone Jan 9, 2019

katbyte merged commit e5c0e2e into hashicorp:master Jan 9, 2019

katbyte added a commit that referenced this pull request Jan 9, 2019

Update CHANGELOG.md to include #2461

ead29e8

hashicorp locked and limited conversation to collaborators Mar 5, 2019


		* `autoscale_formula` - The formula used to autoscale the pool when using scale mode `Auto`.

		* `storage_image_reference` - The reference of the storage image used by the nodes in the Batch pool.

	"resource_group_name": resourceGroupNameDiffSuppressSchema(),
	"resource_group_name": resourceGroupNameForDataSourceSchema(),

Support for Azure Batch - Pool #2461

Support for Azure Batch - Pool #2461

Conversation

jcorioland commented Dec 6, 2018 • edited

tombuildsstuff commented Dec 10, 2018

tombuildsstuff commented Dec 14, 2018

jcorioland commented Dec 14, 2018

katbyte 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcorioland commented Dec 18, 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

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

jcorioland commented Dec 20, 2018 • edited

katbyte commented Dec 21, 2018

jcorioland commented Dec 21, 2018 • edited

tombuildsstuff commented Dec 23, 2018

katbyte left a comment • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcorioland commented Jan 7, 2019

katbyte commented Jan 8, 2019

jcorioland commented Jan 8, 2019

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Mar 5, 2019

jcorioland commented Dec 6, 2018 •

edited

jcorioland commented Dec 20, 2018 •

edited

jcorioland commented Dec 21, 2018 •

edited

katbyte left a comment •

edited