Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/storage zone improvements #1

Conversation

fho
Copy link

@fho fho commented Jun 15, 2022

tests: replace randPullZoneName(), randStorageZoneName() with 1 function

replace the randPullZoneName() and randStorageZoneName() functions that were
doing the same with a function called randResourceName()

-------------------------------------------------------------------------------
readme: mention support for storage zones

-------------------------------------------------------------------------------
tests: add testcase for immutable storagezone fields

Add a testcase that ensures changing an immutable storagezone field fails

-------------------------------------------------------------------------------
examples: add a simple storagezone resource example

-------------------------------------------------------------------------------
simplify storageZoneFromResource

storageZoneFromResource was only setting fields in
*bunny.StorageZoneUpdateOptions that changed in the resource.
Change it to set always all fields to the current values.
This is consistent with how other *FromResource functions are behaving.

The update message sent to the bunny API will contain all settings also the
current ones instead of only the changed ones.
Depending on how the bunny.net endpoint handles missing fields in JSON payloads
for update messages this could be better or worse. :-)
If it always interprets missing fields as no change is done, it causes only
unnecessary network traffic.
If it interprets missing fields as the setting should be unset/disabled, it's
better to include the whole configuration.

The bunny.net API distinguishes between missing and empty string values
for the OriginURL and Custom404FilePath fields. When submitting empty strings
for those it resulted in an error, that those fields set to invalid
values.

To handle it the helper function getOkStrPtr() is introduced. It returns nil if
the value is unset instead of an empty string.

Replacing all get<TYPE>Ptr() calls with getOk<TYPE>Ptr() might make sense and
should be investigated in a follow-up.

-------------------------------------------------------------------------------
storagezone: remove setting state to current values on failed update

It is not needed.
d.Get() only returns the proposed state, if updating fails it is not stored as
current state.

-------------------------------------------------------------------------------
storagezone: remove date_modified key

-------------------------------------------------------------------------------
storagezone: improve error message for immutable fields

- add newlines to make the message easier to read,
- make the message vars constants,

-------------------------------------------------------------------------------
storagezone: fix: creation fails

Creating a storagezone failed with the immutable attribute errors:
        * 'name' is immutable and cannot be changed from '' to 'tf-test-20220615075051824800000019'.[..]
        * 'region' is immutable and cannot be changed from '' to 'DE'. [..]

Fix it by always allowing changes in validateImmutableStringProperty if the old
value was an empty string.

-------------------------------------------------------------------------------

fho added 2 commits June 15, 2022 15:50
Creating a storagezone failed with the immutable attribute errors:
	* 'name' is immutable and cannot be changed from '' to 'tf-test-20220615075051824800000019'.[..]
	* 'region' is immutable and cannot be changed from '' to 'DE'. [..]

Fix it by always allowing changes in validateImmutableStringProperty if the old
value was an empty string.
- add newlines to make the message easier to read,
- make the message vars constants,
@fho fho force-pushed the feat/storage-zone-improvements branch from 3b20b94 to f3188cc Compare June 15, 2022 14:02
fho added 7 commits June 15, 2022 16:12
It is not needed.
d.Get() only returns the proposed state, if updating fails it is not stored as
current state.
storageZoneFromResource was only setting fields in
*bunny.StorageZoneUpdateOptions that changed in the resource.
Change it to set always all fields to the current values.
This is consistent with how other *FromResource functions are behaving.

The update message sent to the bunny API will contain all settings also the
current ones instead of only the changed ones.
Depending on how the bunny.net endpoint handles missing fields in JSON payloads
for update messages this could be better or worse. :-)
If it always interprets missing fields as no change is done, it causes only
unnecessary network traffic.
If it interprets missing fields as the setting should be unset/disabled, it's
better to include the whole configuration.

The bunny.net API distinguishes between missing and empty string values
for the OriginURL and Custom404FilePath fields. When submitting empty strings
for those it resulted in an error, that those fields set to invalid
values.

To handle it the helper function getOkStrPtr() is introduced. It returns nil if
the value is unset instead of an empty string.

Replacing all get<TYPE>Ptr() calls with getOk<TYPE>Ptr() might make sense and
should be investigated in a follow-up.
Add a testcase that ensures changing an immutable storagezone field fails
replace the randPullZoneName() and randStorageZoneName() functions that were
doing the same with a function called randResourceName()
@fho fho force-pushed the feat/storage-zone-improvements branch from f3188cc to ab02902 Compare June 15, 2022 14:12
@fho fho marked this pull request as ready for review June 15, 2022 14:12
@fho
Copy link
Author

fho commented Jun 15, 2022

@jspizziri please review if the changes look fine.
If they do I can merge them with your PR to main.

"make check testacc" succeeded locally.

@@ -256,14 +251,6 @@ func resourceStorageZoneUpdate(ctx context.Context, d *schema.ResourceData, meta

updateErr := clt.StorageZone.Update(ctx, id, storageZone)
if updateErr != nil {
// if our update failed then revert our values to their original
// state so that we can run an apply again.
revertErr := revertUpdateValues(d)

Choose a reason for hiding this comment

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

@fho this is actually currently an important bit, the reason is that there are certain values that never get read out of bunny state on a get (specifically the ones referenced in revertUpdateValues). Additionally, there is validation of these properties that happens on Bunny's side which can fail. If that validation fails on an apply we need to manually revert those values back to what they were before otherwise TF gets confused.

Thats not super clear, so here's a concrete example:

  1. Add custom_404_file_path = "/foo.html" to your resource
  2. Run an apply (this will succeed and correctly modify that value)
  3. Update custom_404_file_path = "/bar" on your resource
  4. Run an apply this apply will fail due to a validation error in Bunny
  5. Run a plan immediately after the failed apply and see that TF now thinks that the state is up-to-date even though it's incorrect as the path is actually /foo.html and not /bar.

To my knowledge, this is an artifact of how those properties can't be read out of Bunny state so we need to infer their state based on successful or errored responses on update.

Hopefully that makes sense. If there's a better way to handle this I'm all ears.

@jspizziri
Copy link

jspizziri commented Jun 15, 2022

Another point I want to bring up. I have no idea what OriginURL does on a SZ... it doesn't show up anywhere in the UI and seems only settable via the API. I'm suspicious that it's actually a mistake. There doesn't even seem to be any URL validation on it on Bunny's side (you can pass in "hello" or anything else you like including an empty string and Bunny will say it's fine).

I suppose it's in the API so we should support it?

@jspizziri
Copy link

I'm going to merge your changes in and I'll make my modifications on top of them.

@jspizziri jspizziri merged commit e36ebef into 5-stones:feat/storage-zone Jun 15, 2022
@fho
Copy link
Author

fho commented Jun 15, 2022

Another point I want to bring up. I have no idea what OriginURL does on a SZ... it doesn't show up anywhere in the UI and seems only settable via the API. I'm suspicious that it's actually a mistake. There doesn't even seem to be any URL validation on it on Bunny's side (you can pass in "hello" or anything else you like including an empty string and Bunny will say it's fine).

I suppose it's in the API so we should support it?

Interesting, we can ask the bunny support if the field has any functionality. In the past they always replied very quickly.
Let me know if you contact them.
Otherwise I can do it tomorrow.

@jspizziri
Copy link

@fho good idea. I'll contact them and let you know the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants