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

[resource] add 'next-pool' field support to Pool #106

Merged
merged 4 commits into from
Sep 28, 2022

Conversation

maksym-nazarenko
Copy link
Collaborator

This PR adds support of next-pool field to the pool resource.
Closes #23

@maksym-nazarenko maksym-nazarenko marked this pull request as ready for review August 19, 2022 21:15
@maksym-nazarenko
Copy link
Collaborator Author

@ddelnano during investigation, I found that client.Marshal() func skips empty string so it is not possible to set any string value to an empty string via API.

fix identation

Co-authored-by: Dom Del Nano <ddelnano@gmail.com>
@ddelnano
Copy link
Owner

@maksym-nazarenko apologies for the long delay on this. I was traveling and got engaged.

during investigation, I found that client.Marshal() func skips empty string so it is not possible to set any string value to an empty string via API.

Regarding that, maybe the Marshal interface should allow something like omitempty like the json package provides? That would allow us to disambiguate a case like this, right?

I'll merge this for now, but I'd like to here your thoughts about the above.

@ddelnano ddelnano merged commit 7cbf55a into ddelnano:master Sep 28, 2022
@maksym-nazarenko
Copy link
Collaborator Author

apologies for the long delay on this

no worries Dom, I found you work really useful so I'm happy to help you with enhancement of this provider

I was traveling and got engaged

my congratulations to you 🎉

Regarding that, maybe the Marshal interface should allow something like omitempty like the json package provides? That would allow us to disambiguate a case like this, right?

yes, in theory, but sometimes you mean omitempty to skip this field entirely during Update and sometimes you want exactly "" (empty string) to be set.
Frankly speaking, I have no good idea how to distinguish those to cases at the moment 🤷
Anyway, let's keep it in mind when touch Marshal next time

@ddelnano
Copy link
Owner

I really appreciate your work on this and so just want to continue to make sure that my slow replies don't make you feel that the changes aren't appreciated! And thanks for the congratulations!

Yep, that makes sense and I agree we can keep that in mind for the future 👍

@maksym-nazarenko maksym-nazarenko deleted the feature/next-pool-field branch September 19, 2023 18:53
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.

Next-Pool field unable to be defaulted to 'none' on IP Pool creation
2 participants