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.
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
Add support for Get/Patch API Shield API Discovery Operations #1413
Add support for Get/Patch API Shield API Discovery Operations #1413
Changes from 1 commit
01e4f42
3aa0da5
80d9650
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.
Should we provide the set of valid state strings as consts that one can pass here?
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.
entirely up to you folks. we do half and half in other places but once we do SDK generation, we will have allowed enums.
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.
Have opted to add typed field + consts for these fields 80d9650
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.
Why is this not a uuid?
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.
we pass string for the ID field in normal operations too https://github.com/cloudflare/cloudflare-go/blob/master/api_shield_operations.go#L13-L18
I'm not sure if it's a good idea to enforce the type here or not (we'd need cloudflare-go to depend on a uuid library like
github.com/google/uuid
)@jacobbednarz what do you think? I'm inclined to just stick with string. If a non uuid is provided then the API will respond with an invalid input error
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.
string seems fine to me here.
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.
instead of
Patch
, we prefer to useUpdate
unless the HTTP method is important context to have for the caller. more details in https://github.com/cloudflare/cloudflare-go/blob/master/docs/experimental.md#consistent-crud-method-signaturesThere 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.
makes sense to me - thanks!
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.
Just to check: Does the
.makeRequestContext
function add the respective request body and corresponding headers?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.
Does it know that it should only add the state field and not the operation ID to the request body?
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.
Or should we rather pass the UpdateAPIShieldDiscoveryOperation type here?
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.
yes, the
params
(of typeUpdateAPIShieldDiscoveryOperationParams
) is what gets put in the body.The
params
struct is serialized to JSON herecloudflare-go/cloudflare.go
Lines 222 to 227 in 217beeb
and the
content-type
header is set toapplication/json
if none is set herecloudflare-go/cloudflare.go
Lines 382 to 384 in 217beeb
The operation ID does not get serialized as we set the json tag to
-
https://github.com/cloudflare/cloudflare-go/pull/1413/files#diff-a8b0a1c475237e4f8ba6786369d22fa9008203858112922a8a8c1426ce423e6dR60