Add PublicKey field to S3 all CRUD actions #198

mccurdyc · 2020-06-03T15:47:39Z

Proposed Change(s)

Add missing PublicKey field to Creates and Updates and responses.
Update integration test mock recordings for S3.

Relevant Link(s) / Documentation

API Docs

Validation

$ export FASTLY_TEST_SERVICE_ID=<foo>; \
export FASTLY_API_KEY="$(stoml ~/.config/fastly/config.toml token)"; \
rm -rf fastly/fixtures/s3* && \
make test TESTARGS="-run=S3 -count=1" && \
make fix-fixtures && \
unset FASTLY_TEST_SERVICE_ID FASTLY_API_KEY && \
make test

…field Signed-off-by: Colton J. McCurdy <cmccurdy@fastly.com>

Signed-off-by: Colton J. McCurdy <cmccurdy@fastly.com>

mccurdyc · 2020-06-03T16:08:12Z

fastly/fastly_test.go

@@ -290,6 +290,68 @@ wMfrTEOvx0NxUM3rpaCgEmuWbB1G1Hu371oyr4srrr+N
 `
 }

+// pgpPublicKeyUpdate returns a PEM encoded PGP public key suitable for testing
+// updates to the public key field.
+func pgpPublicKeyUpdate() string {


I'm "iffy" on doing this.

Since there are fake keys all over the test suite, this doesn't bother me too much. Worth adding to the list of testing cleanup TODOs, though.

What are your concerns?

My main concern was just introducing another test helper that just returned some mocked data.

mccurdyc · 2020-06-03T16:09:34Z

fastly/fixtures/s3s/create.yaml

@@ -30,6 +30,38 @@ interactions:
      - "12"
      placement:
      - waf_debug
+      public_key:
+      - |


This is new

mccurdyc · 2020-06-03T16:10:11Z

fastly/fixtures/s3s/update.yaml

      name:
      - new-test-s3
+      public_key:


This is the new field

mccurdyc · 2020-06-03T16:11:26Z

fastly/s3_test.go

@@ -27,10 +27,12 @@ func TestClient_S3s(t *testing.T) {
 			GzipLevel:                    9,
 			Format:                       "format",
 			FormatVersion:                2,
+			ResponseCondition:            "",


Explicitly setting zero value just to show that it is covered.

However without adding additional setup --- i.e., creating a valid response condition --- we can't set a value here.

Maybe a comment?

The coverage here seems fine to me. If you think a comment will help highlight your concern in the future, go for it.

I think this is fin as-is

mccurdyc · 2020-06-03T16:11:44Z

fastly/s3_test.go

+			Version:   tv.Number,
+			Name:      "test-s3",
+			NewName:   "new-test-s3",
+			PublicKey: pgpPublicKeyUpdate(),


update public key

phamann

👍 LGTM

phamann · 2020-06-03T16:47:14Z

fastly/fastly_test.go

@@ -290,6 +290,68 @@ wMfrTEOvx0NxUM3rpaCgEmuWbB1G1Hu371oyr4srrr+N
 `
 }

+// pgpPublicKeyUpdate returns a PEM encoded PGP public key suitable for testing
+// updates to the public key field.
+func pgpPublicKeyUpdate() string {


What are your concerns?

phamann · 2020-06-03T16:49:54Z

fastly/s3_test.go

@@ -27,10 +27,12 @@ func TestClient_S3s(t *testing.T) {
 			GzipLevel:                    9,
 			Format:                       "format",
 			FormatVersion:                2,
+			ResponseCondition:            "",


I think this is fin as-is

mccurdyc added 2 commits June 3, 2020 11:42

helpers: add a test helper for testing updates to the PGP public key …

6859efb

…field Signed-off-by: Colton J. McCurdy <cmccurdy@fastly.com>

s3: add supported PublicKey field to Updates.

d5da8d4

Signed-off-by: Colton J. McCurdy <cmccurdy@fastly.com>

mccurdyc requested review from ezkl and phamann June 3, 2020 15:47

mccurdyc added the enhancement New feature or request label Jun 3, 2020

mccurdyc commented Jun 3, 2020

View reviewed changes

fastly/fixtures/s3s/update.yaml

name:

- new-test-s3

public_key:

Copy link

Contributor Author

mccurdyc Jun 3, 2020

Choose a reason for hiding this comment

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

This is the new field

mccurdyc commented Jun 3, 2020

View reviewed changes

mccurdyc marked this pull request as ready for review June 3, 2020 16:12

mccurdyc changed the title ~~Add PublicKey field to S3 Updates~~ Add PublicKey field to S3 all CRUD actions Jun 3, 2020

ezkl approved these changes Jun 3, 2020

View reviewed changes

phamann approved these changes Jun 3, 2020

View reviewed changes

mccurdyc merged commit c4599a1 into fastly:master Jun 3, 2020

mccurdyc deleted the mccurdyc/s3-publickey-all branch June 3, 2020 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PublicKey field to S3 all CRUD actions #198

Add PublicKey field to S3 all CRUD actions #198

mccurdyc commented Jun 3, 2020 •

edited

Loading

mccurdyc Jun 3, 2020

ezkl Jun 3, 2020

phamann Jun 3, 2020

mccurdyc Jun 3, 2020

mccurdyc Jun 3, 2020 •

edited

Loading

mccurdyc Jun 3, 2020

mccurdyc Jun 3, 2020

ezkl Jun 3, 2020

phamann Jun 3, 2020

mccurdyc Jun 3, 2020

phamann left a comment

phamann Jun 3, 2020

phamann Jun 3, 2020

Add PublicKey field to S3 all CRUD actions #198

Add PublicKey field to S3 all CRUD actions #198

Conversation

mccurdyc commented Jun 3, 2020 • edited Loading

Proposed Change(s)

Relevant Link(s) / Documentation

Validation

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

mccurdyc Jun 3, 2020 • edited Loading

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

phamann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccurdyc commented Jun 3, 2020 •

edited

Loading

mccurdyc Jun 3, 2020 •

edited

Loading