OC Migrate Blocks to using AttributeHandlers #267

ghost · 2020-06-19T14:05:07Z

No description provided.

…d). No Read function for ElasticSearch and FTP to align with master

…throwing errors

mccurdyc

LGTM! This is amazing! This pattern will certainly improve maintainability and adding new resources.

The only action items:

Consider making MustProcess method more dynamic by having a more general param than initialVersion bool
Use GetKey pattern ubiquitously in logging endpoints versus the hardcoded string.
Move test fixtures to a test-fixtures sub-directory.
Update test helper for reading test fixtures to include a *testing.T param and call t.Fatal instead of panic.

fastly/resource_fastly_service_v1.go

fastly/block_fastly_service_v1_bigquerylogging.go

fastly/resource_fastly_service_v1.go

fastly/base_fastly_service_v1.go

mccurdyc · 2020-06-19T14:56:05Z

fastly/base_fastly_service_v1.go

+func (h *DefaultServiceAttributeHandler) HasChange(d *schema.ResourceData) bool {
+	return d.HasChange(h.key)
+}
+
+func (h *DefaultServiceAttributeHandler) MustProcess(d *schema.ResourceData, initialVersion bool) bool {
+	return h.HasChange(d)
+}


Not blocking.

Comments on these two functions would be really appreciated because they are very similar and it might not be clear when to use one or the other.

fastly/block_fastly_service_v1_s3logging.go

fastly/fastly_test.go

mccurdyc · 2020-06-19T15:53:36Z

fastly/fastly_test.go

@@ -230,3 +94,7 @@ func TestEscapePercentSign(t *testing.T) {
 		})
 	}
 }
+
+func appendNewLine(s string) string {


If this isn't used anymore (I don't think it should be), we can delete it.

fastly/fastly_test.go

fastly/resource_fastly_service_v1.go

ghost · 2020-06-19T16:27:12Z

Resolves:
2. Use GetKey pattern ubiquitously in logging endpoints versus the hardcoded string.

ghost · 2020-06-19T16:32:59Z

Resolves:
3. Move test fixtures to a test-fixtures sub-directory.

ghost · 2020-06-19T16:45:04Z

Resolves:
4. Update test helper for reading test fixtures to include a *testing.T param and call t.Fatal instead of panic.

mccurdyc · 2020-06-23T12:34:52Z

fastly/block_fastly_service_v1_logging_googlepubsub.go

 }

-func processGooglePubSub(d *schema.ResourceData, conn *gofastly.Client, latestVersion int) error {
+func (h *GooglePubSubServiceAttributeHandler) Process(d *schema.ResourceData, latestVersion int, conn *gofastly.Client) error {
 	serviceID := d.Id()
 	oldLogCfg, newLogCfg := d.GetChange("logging_googlepubsub")


Suggested change

oldLogCfg, newLogCfg := d.GetChange("logging_googlepubsub")

oldLogCfg, newLogCfg := d.GetChange(h.GetKey())

mccurdyc · 2020-06-23T12:36:20Z

fastly/block_fastly_service_v1_logging_scalyr.go

 }

-func processScalyr(d *schema.ResourceData, conn *gofastly.Client, latestVersion int) error {
+func (h *ScalyrServiceAttributeHandler) Process(d *schema.ResourceData, latestVersion int, conn *gofastly.Client) error {
 	serviceID := d.Id()
 	oldLogCfg, newLogCfg := d.GetChange("logging_scalyr")


Suggested change

oldLogCfg, newLogCfg := d.GetChange("logging_scalyr")

oldLogCfg, newLogCfg := d.GetChange(h.GetKey())

mccurdyc · 2020-06-23T12:39:14Z

fastly/test_fixtures/fastly_test_publickey

@@ -0,0 +1,53 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----


You may have to update this file once you rebase master.

mccurdyc · 2020-06-23T12:41:36Z

~~1. Consider making MustProcess method more dynamic by having a more general param than initialVersion bool~~
- We discussed offline. We will make this change when it is necessary as changing the API should be safe.
2. Use GetKey pattern ubiquitously in logging endpoints versus the hardcoded string.
3. Move test fixtures to a test-fixtures sub-directory.
4. Update test helper for reading test fixtures to include a *testing.T param and call t.Fatal instead of panic.

Everything looks good to me! I saw that googlepubsub and scalyr did still need to be updated in one place to use the GetKey pattern.

Guy Richardson added 30 commits June 19, 2020 14:46

Added missing readFTP and readElasticsearch functions

d367e63

Removed readFTP and readElasticsearch to align with master

d3fdfe1

Migrated bigquerylogging to attribute handler

7978ed7

Migrated blobstoragelogging to attribute handler

a64d58f

Migrated cachesetting to attribute handler

ab52a7b

Migrated condition to attribute handler

81992eb

Migrated dictionary to attribute handler

86317f4

Migrated director to attribute handler

7c959d6

Migrated domain to attribute handler

f4b905b

Migrated dynamicsnippet to attribute handler

96ab33a

Incorrect key for domain

c5e34ef

Migrated gcslogging to attribute handler

4a53427

Migrated gzip to attribute handler

7be4498

Migrated header to attribute handler

8780f90

Migrated healthcheck to attribute handler

e0ec7f6

Migrated httpslogging to attribute handler

d11ca5f

Migrated logentries to attribute handler

4778654

Migrated papertrail to attribute handler

5e7f027

Migrated requestsetting to attribute handler

add2e3b

Migrated responseobject to attribute handler

44c81c1

Migrated s3logging to attribute handler

23d05d5

Migrated snippet to attribute handler

83661e3

Migrated splunk and sumologic to attribute handler

e3bcde4

Sumologic error

47d0808

Migrated syslogic to attribute handler

053a364

Migrated vcl to attribute handler

7578ae9

Tidy up

dbb7bf9

Refactored to make a bit more DRY

5e6c3ef

make fmt

7b4c256

Refactored ElasticSearch & FTP to handler (and ACL which had regresse…

479f78f

…d). No Read function for ElasticSearch and FTP to align with master

Guy Richardson added 14 commits June 19, 2020 14:46

Removed WASM definition from this branch

725e5a9

make fmt

153520f

Minor fixes to cherry pick

d9aa6f4

Refactored documentation

dd39a25

Matched up default format to tests

e38240e

Fixed some of the tests - migrated certs to their own file since EOF …

332a4f4

…throwing errors

Renamed AttributeHandlers

49be303

Updated comments

39c9637

Converted over new logging blocks to Handlers

5d1baff

Refactored to changes to S3 logging

053e1bf

Tidy up from commit history

8a0f559

Migrated Scalyr and GooglePubSub to handlers

746c1c0

Migrated from EOF to Terraform file command

b6eecea

make fmt

53f1b97

ghost added the size/XXL label Jun 19, 2020

Migrated Kafka Logging over to AttributeHandler

cfcd0f5

mccurdyc approved these changes Jun 19, 2020

View reviewed changes

Migrated literal block keys to h.GetKey()

d159492

Migrated keys to test_fixtures directory

78a3f5d

Added t *testing.T to key functions to bind into test process

bb03b73

Fixed Fatalf error

46ed794

mccurdyc reviewed Jun 23, 2020

View reviewed changes

ghost closed this Jun 23, 2020

ghost deleted the oc/block_handlers branch June 23, 2020 15:13

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OC Migrate Blocks to using AttributeHandlers #267

OC Migrate Blocks to using AttributeHandlers #267

ghost commented Jun 19, 2020

mccurdyc left a comment •

edited

Loading

mccurdyc Jun 19, 2020

mccurdyc Jun 19, 2020

ghost commented Jun 19, 2020

ghost commented Jun 19, 2020

ghost commented Jun 19, 2020

mccurdyc Jun 23, 2020

mccurdyc Jun 23, 2020

mccurdyc Jun 23, 2020

mccurdyc commented Jun 23, 2020 •

edited

Loading

	oldLogCfg, newLogCfg := d.GetChange("logging_googlepubsub")
	oldLogCfg, newLogCfg := d.GetChange(h.GetKey())

OC Migrate Blocks to using AttributeHandlers #267

OC Migrate Blocks to using AttributeHandlers #267

Conversation

ghost commented Jun 19, 2020

mccurdyc left a comment • edited Loading

Choose a reason for hiding this comment

mccurdyc Jun 19, 2020

Choose a reason for hiding this comment

mccurdyc Jun 19, 2020

Choose a reason for hiding this comment

ghost commented Jun 19, 2020

ghost commented Jun 19, 2020

ghost commented Jun 19, 2020

mccurdyc Jun 23, 2020

Choose a reason for hiding this comment

mccurdyc Jun 23, 2020

Choose a reason for hiding this comment

mccurdyc Jun 23, 2020

Choose a reason for hiding this comment

mccurdyc commented Jun 23, 2020 • edited Loading

mccurdyc left a comment •

edited

Loading

mccurdyc commented Jun 23, 2020 •

edited

Loading