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

Ensure all relevant 'create' input fields are using pointers. #382

Merged
merged 51 commits into from Nov 7, 2022

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Nov 3, 2022

NOTE: To review this PR I would recommend looking at each individual commit rather than the "Files changed" view.


To avoid issues where a user wants to set a zero value on a create input field (and to avoid the API potentially setting an unwanted default value) we will convert all relevant fields to pointers.

The intention is that if the user doesn't pass a pointer for one of these fields, then the data won't be marshalled and will be omitted from the request to the API. If the user sends a zero value for the field, then because it will have to be wrapped in a pointer, it means the zero value for the given field type will be sent in the request to the API.

At that point it's up to the API to either respect that zero value or not, and in some cases we know the API actually ignores the zero value and will set a default. regardless.

Additional work

I've ensured that integers use int rather than uint following the Golang guidance...

When you need an integer value you should use int unless you have a specific reason to use a sized or unsigned integer type.

...although we know some fields are always going to be positive numbers, using int is more consistent and more flexible (if the API ever needed to change), as those types of changes require tedious updates to the Fastly CLI and Fastly Terraform provider.

Other comments

Not all create operations are going to have all fields converted to pointers.

If we consider the 'token' creation endpoint (which is fastly.CreateToken()), there is no point in making a field such as Services a pointer because omitting that field will cause the API to set the token to have access to all services, and if that is undesired behaviour the user would set some form of value. Therefore, trying to allow setting of an empty value makes no sense because you would never want to create a token that didn't have access to at least a single service.

JSON API endpoints have mostly been left untouched as their semantics are a little less clear and due to complexity in their setup (e.g. TLS and WAF products) I'm not looking to mess around with those at this point in time.

@Integralist Integralist force-pushed the integralist/create-pointers branch 2 times, most recently from 0b4ba41 to ad0252c Compare November 3, 2022 18:04
@Integralist Integralist marked this pull request as ready for review November 7, 2022 15:11
@Integralist Integralist merged commit 9261ce9 into main Nov 7, 2022
@Integralist Integralist deleted the integralist/create-pointers branch November 7, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant