feat(storagezone): add the ability to add, get, list, and remove stor… #19

jspizziri · 2022-05-28T19:26:38Z

…age zones

jspizziri · 2022-05-28T19:30:54Z

My overarching goal is to get storage zone management into the TF provider you've written. My Golang-foo is weak (this is in fact the first Go I've ever written. There's a lot of repetition from the pullzone code and I think a more generic resource that has abstractions for Add, Get, List, Pagination, etc would be best. I took a look at Go generics and played around with them for a few minutes, and couldn't immediately get them to work for me. I figured I'd push this up as something that works is more valuable than something that's better but ultimately doesn't exist.

Would be interested in your thoughts and guidance.

fho

@jspizziri thanks a lot for your contribution!
It looks great overall.

Please see my review comments + also check out the warnings from the CI check.

storagezone_test.go

storagezone_list.go

storagezone_add.go

jspizziri · 2022-05-31T14:53:16Z

@fho please see my updates. I believe I've taken care of everything you mentioned. I refactored things to use functions with generics and I had to upgrade to go 1.18 to do so. LMK what you think.

fho · 2022-05-31T15:14:25Z

@jspizziri CI is failing because it uses Go 1.17 (needs to be upgraded in .github/workflows/ci.yml).

I would prefer if you could split those changes into multiple PRs or at least multiple commits.
Updating Go, supporting the storage API and refactoring the code to generics are separate topics.

It makes it easier to use the commit messages as base for changelogs, enables to revert specific changes that have introduced bugs, makes it easier to review PRs and it's also quicker to finish small changes and get them into master. :-)

jspizziri · 2022-05-31T15:26:34Z

@fho please see my updates.

jspizziri · 2022-05-31T15:46:00Z

@fho the workflow is failing due to the linter. I checked and it looks like golangci-lint might not support go 1.18 yet. I did see references to go 1.18 in the changelog however, so I just pushed an update to the version of the linter the workflow is using. We'll see what happens.

fho · 2022-05-31T15:53:39Z

@jspizziri yes, the recent golangci-lint versions work with go 1.18 but some linters are still unsupported and get disabled when requiring go 1.18

jspizziri · 2022-05-31T16:19:40Z

@fho alright, all the linter errors are cleaned up and the workflow is passing.

fho · 2022-05-31T16:25:22Z

@jspizziri awesome, thanks a lot.

I like the idea with the generic resource functions!
I'll review it more thoroughly latest on Thursday, have to read a bit about go-generics again first, haven't used them so far. :-)

jspizziri · 2022-05-31T20:51:34Z

FYI I'm adding update as well since it's required by TF. I'll let you know once it's there, but it will be almost identical to everything else in terms of design so please still take a look at the generics usage.

jspizziri · 2022-06-01T16:45:59Z

Alright, I've updated the client to add update to storage zones. A few things of note:

I modified the client to check for a 204 status code and not require a response (204 is an empty response code).
There was a notable naming inconsistency with the ReplicationRegions/ReplicationZones property.

fho

looks great, I added some comments

client.go

resource_add.go

resource_list.go

resource_update.go

storagezone_add.go

storagezone_get.go

storagezone_update.go

fho · 2022-06-03T07:45:54Z

@jspizziri there are multiple mehtods that do not use the new resource functions, e.g.: AddCustomCertificate, RemoveCustomHostname, SetEdgeRuleEnabled could you change all to use the new resource functions, so it is consistent?

fho · 2022-06-03T10:49:57Z

@jspizziri fyi, I made https://github.com/simplesurance/bunny-cli public.
I created the tool to test the bunny-go package manually and make experimenting with the bunny-go api a bit easier.
Often using curl + jq works similar well and testing bunny-go is better done with testcases. :-)
Maybe it's still helpful to you

jspizziri · 2022-06-03T13:47:04Z

I've pushed a new update. Some things of note:

Refactored all the pullzone functions to use the abstracted resource* functions
In doing so, I had to abstract the resource functions a bit more (pulled path interpolation out and added some more options)
I removed resource_add and resource_update in favor of a resource_post which has functions for getting a response with & without content. Bunny doesn't really have a true update/PUT endpoint, so it reflects more closely their actual API.
The LoadFreeCertificate function seemed a bit special so I didn't touch that for now.

LMK your thoughts from here

…ete storage zones

jspizziri · 2022-06-03T19:44:48Z

Also, something I want to note that I just discovered about on PullZones:

The docs say that on creation of a PZ an OriginURL is required and a StorageZoneId is optional. However, the reality is that ONE of the TWO are required. You must either specify a OriginURL OR a StorageZoneID. Furthermore, passing BOTH results in OriginURL being entirely ignored (StorageZoneID is preferred).

I found this while updating the TF provider to support Storage Zones. I feel that the libs should help in this regard, and my thought was that bunny-go could log a warning if you pass both at the same time and that the TF provider could throw an error if you pass both at the same time (since it creates issues with the TF diff make it seem like there are changes to apply when there are not)

Update: I've addressed this issue in the TF provider here simplesurance/terraform-provider-bunny#67, but I think there should be something in this package that at least warns people.

jspizziri · 2022-06-06T13:59:37Z

Also, it should be noted that the refactor of pagination actually introduces a breaking change since it removes some old structs in favor of abstract ones. I've updated the commit dealing with that refactor/feature to document the breaking change.

…esource functions upgrade pullzone to take advantaged of the abstract resource functions. a breaking change has been introduced in this version by removing PullZone specific pagination structs in favor of more abstract ones. BREAKING CHANGE: `PullZonePaginationOptions` and `PullZonePaginationReply` have been replaced with `PaginationOptions` and `PaginationReply`

fho · 2022-06-07T10:44:44Z

The docs say that on creation of a PZ an OriginURL is required and a StorageZoneId is optional. However, the reality is that ONE of the TWO are required. You must either specify a OriginURL OR a StorageZoneID. Furthermore, passing BOTH results in OriginURL being entirely ignored (StorageZoneID is preferred).

nice find, lets document it in the godoc for PullZoneAddOptions and log a message if both are passed as you suggested in a separate PR

fho · 2022-06-07T11:06:37Z

@jspizziri looks good! thanks a for the work again.

I'm in favor of removing resourceUpdate204 and pass resp to resourcePost explicitly. I think it's more simple and clear.
I also can do that before I merge the PR.

My plan is to fix the failing integrationtest in the bunny-terraform-go provider, then run the the terraform provider with this bunny-go version to test the change thoroughly. I expect that everything works fine but better be safe. :-)
Afterwards I'll merge it.

jspizziri · 2022-06-07T13:23:54Z

The docs say that on creation of a PZ an OriginURL is required and a StorageZoneId is optional. However, the reality is that ONE of the TWO are required. You must either specify a OriginURL OR a StorageZoneID. Furthermore, passing BOTH results in OriginURL being entirely ignored (StorageZoneID is preferred).

nice find, lets document it in the godoc for PullZoneAddOptions and log a message if both are passed as you suggested in a separate PR

#24

jspizziri · 2022-06-07T13:35:29Z

I also can do that before I merge the PR.

@fho if you're willing to do that I'd appreciate it. I'm quite anxious to get my hands on that tf provider as I'm sure you can imagine and whatever makes that happen the fastest is my preference (and if you do it we won't need to go through another review cycle).

The docs say that on creation of a PZ an OriginURL is required and a StorageZoneId is optional. However, the reality is that ONE of the TWO are required. You must either specify a OriginURL OR a StorageZoneID. Furthermore, passing BOTH results in OriginURL being entirely ignored (StorageZoneID is preferred). (simplesurance/bunny-go#19 (comment))

This merge: - updates the minimum go version to 1.18, - updates golangci-lint in CI, - add support for retrieving, adding, updating and deleting storage zones - introduces resource* functions that combine calling new*Request and sendRequest, - fixes typos in some error messages GitHub Pull-Request #19

fho · 2022-06-07T17:13:09Z

@jspizziri my idea to of passing resp directly did not work as I thought because I still had the type parameter I could not pass nil.

I merged with some small changes on top, thanks a lot for the PR!

jspizziri · 2022-06-07T17:22:44Z

@fho thanks! Thanks for all the work on this lib and the tf provider. Definitely going to be super helpful for us.

Have you reached out to the Bunny folks to let them know about these tools? I feel like a lot of people could use the TF provider and they may want to list it here

fho · 2022-06-08T07:26:47Z

@jspizziri yes, they are aware of both. I also told them that it would be great if they could list them on their 3. party tools page but it did not happen 🤷.

fho reviewed May 30, 2022

View reviewed changes

storagezone_test.go Outdated Show resolved Hide resolved

storagezone_list.go Outdated Show resolved Hide resolved

storagezone_add.go Outdated Show resolved Hide resolved

jspizziri force-pushed the feat/storage-zone branch from 5f5ae25 to 0b1263c Compare May 31, 2022 14:44

jspizziri requested a review from fho May 31, 2022 14:53

jspizziri force-pushed the feat/storage-zone branch from 1ea304d to 84cdfd4 Compare May 31, 2022 15:26

build(go): upgrade go from 1.17 to 1.18

61975e2

jspizziri force-pushed the feat/storage-zone branch from 84cdfd4 to c24ca2e Compare May 31, 2022 15:47

jspizziri force-pushed the feat/storage-zone branch 2 times, most recently from bc57b8f to 1fc8d8d Compare May 31, 2022 16:17

jspizziri force-pushed the feat/storage-zone branch from 1fc8d8d to fbca6da Compare June 1, 2022 16:41

fho reviewed Jun 2, 2022

View reviewed changes

jspizziri force-pushed the feat/storage-zone branch 4 times, most recently from cb342b4 to 10efbb0 Compare June 2, 2022 21:23

jspizziri force-pushed the feat/storage-zone branch 3 times, most recently from 8d0d4b6 to 8e4b532 Compare June 3, 2022 13:43

jspizziri added 2 commits June 3, 2022 08:58

feat(storagezone): add the ability to get, list, add, update, and del…

61d3d82

…ete storage zones

test(integrationtest): fix typo in integration test

a2d8753

jspizziri force-pushed the feat/storage-zone branch 2 times, most recently from 12d0ead to 436d471 Compare June 3, 2022 14:05

jspizziri mentioned this pull request Jun 4, 2022

feat(pullzone): pullzone origins (OriginURL & StorageZoneID) are mutu… simplesurance/terraform-provider-bunny#67

Merged

jspizziri requested a review from fho June 6, 2022 16:55

jspizziri force-pushed the feat/storage-zone branch from 436d471 to 5d45067 Compare June 6, 2022 17:02

This was referenced Jun 7, 2022

Add constants for Region & ReplicationRegion #23

Open

Add warning & documentation for mutually exclusive Origin properties #24

Open

fho merged commit 5d45067 into simplesurance:main Jun 7, 2022

jspizziri deleted the feat/storage-zone branch June 7, 2022 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storagezone): add the ability to add, get, list, and remove stor… #19

feat(storagezone): add the ability to add, get, list, and remove stor… #19

jspizziri commented May 28, 2022

jspizziri commented May 28, 2022

fho left a comment

jspizziri commented May 31, 2022

fho commented May 31, 2022

jspizziri commented May 31, 2022

jspizziri commented May 31, 2022 •

edited

Loading

fho commented May 31, 2022

jspizziri commented May 31, 2022

fho commented May 31, 2022

jspizziri commented May 31, 2022

jspizziri commented Jun 1, 2022

fho left a comment

fho commented Jun 3, 2022

fho commented Jun 3, 2022

jspizziri commented Jun 3, 2022 •

edited

Loading

jspizziri commented Jun 3, 2022 •

edited

Loading

jspizziri commented Jun 6, 2022 •

edited

Loading

fho commented Jun 7, 2022

fho commented Jun 7, 2022

jspizziri commented Jun 7, 2022

jspizziri commented Jun 7, 2022

fho commented Jun 7, 2022

jspizziri commented Jun 7, 2022 •

edited

Loading

fho commented Jun 8, 2022

feat(storagezone): add the ability to add, get, list, and remove stor… #19

feat(storagezone): add the ability to add, get, list, and remove stor… #19

Conversation

jspizziri commented May 28, 2022

jspizziri commented May 28, 2022

fho left a comment

Choose a reason for hiding this comment

jspizziri commented May 31, 2022

fho commented May 31, 2022

jspizziri commented May 31, 2022

jspizziri commented May 31, 2022 • edited Loading

fho commented May 31, 2022

jspizziri commented May 31, 2022

fho commented May 31, 2022

jspizziri commented May 31, 2022

jspizziri commented Jun 1, 2022

fho left a comment

Choose a reason for hiding this comment

fho commented Jun 3, 2022

fho commented Jun 3, 2022

jspizziri commented Jun 3, 2022 • edited Loading

jspizziri commented Jun 3, 2022 • edited Loading

jspizziri commented Jun 6, 2022 • edited Loading

fho commented Jun 7, 2022

fho commented Jun 7, 2022

jspizziri commented Jun 7, 2022

jspizziri commented Jun 7, 2022

fho commented Jun 7, 2022

jspizziri commented Jun 7, 2022 • edited Loading

fho commented Jun 8, 2022

jspizziri commented May 31, 2022 •

edited

Loading

jspizziri commented Jun 3, 2022 •

edited

Loading

jspizziri commented Jun 3, 2022 •

edited

Loading

jspizziri commented Jun 6, 2022 •

edited

Loading

jspizziri commented Jun 7, 2022 •

edited

Loading