This repository has been archived by the owner on Jul 27, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(bunny_pullzone): add a datasource for pull zones #98
base: main
Are you sure you want to change the base?
feat(bunny_pullzone): add a datasource for pull zones #98
Changes from all commits
1f0bb76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the usecase for keyZoneSecurityEnabled and keyZoneSecurityIncludeHashRemoteIP, when managing a videolib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are properties that, while exposed via the Video Library update endpoint, are not fetchable as part of the Video Library, as they reside exclusively on the Pull Zone. I put them in here as I think they'll be helpful in certain cases to determine the full state of a video library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the
EnableTokenIPVerification
andPlayerTokenAuthenticationEnabled
fields in the videolib update call change thekeyZoneSecurityEnabled
andkeyZoneSecurityIncludeHashRemoteIP
settings in the pull-zone? 👀There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, those are pass throughs that bunny internally sets on the managed pull-zone. The unfortunate part is that they don't also return them in the video library API. I asked the Bunny team about this in a support request and they said you have to query the Pull Zone directly to get them. The other alternative to this is to do a special PullZone.Get as part of the VideoLibrary update process, which doesn't feel great to me. Exposing them in this way seemed fairly simple, although it has the downside of your
bunny_videolibrary
resource state potentially being incorrect if something weird happens.I do like bunny a lot, but when they do stuff like this I get a bit frustrated with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum, I see, that makes it complicated.
Having to use fields in a pullzone datasource as workaround for the issue makes the Terraform provider difficult to use.
Users would need to know these API internals, that a videolib has an underlying pullzone, that some videolib attributes are stored in the pullzone and also the different attribute names.
A simple solution in other cases were a field was not retrievable via the Get endpoint was to mark the resource attribute as
ForceNew: true
.I don't know if that is feasible for those videolib attributes.
Would be having to recreated the videolib when these attributes are changed an option?
Otherwise I think it is better to do the additional pullzone get call in the
ReadContext
function of the videolib.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ForceNew isn't really an option. It would delete all of your video content. As of the current state of the videolibrary resource I did something similar to what we did on the custom_404 property for storage zones if you recall that. Effectively, if the API call succeeds we just update the state to whatever value was sent to bunny. Which seems finish... until I found a bug in the bunny API where they were silently failing to set those properties in certain circumstances (I filed a bug report with them and they're going to fix it).
But still, stuff like that can happen unless you do a hard read on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if we're going to read the pullzone in the bunny videolibrary resource, then it seems like we might as well return the entire data set that we care about from it instead of doing a datasource. The data source seemed like a cleaner way at first, but maybe that should be considered instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go for that solution.
Hiding these ugly internals to make the provider easier to use will be worth the additional api requests and code.