-
Notifications
You must be signed in to change notification settings - Fork 817
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
Refactored sdk functions to always return &alpha.Bool{} instead of nil #1958
Refactored sdk functions to always return &alpha.Bool{} instead of nil #1958
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Build Failed 😱 Build Id: 1f1b0dbd-2b59-481b-a833-12a2d508e521 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
e0e60b2
to
eec8942
Compare
Build Succeeded 👏 Build Id: 1750ee46-67f9-4c88-b1ec-93a5b9c42947 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Thanks for this! Can we switch This is a great first start, but we'll likely also want at least an e2e test to confirm that this actually closes the bug in question, so we won't want to close the issue until that is done. Unless you want to add an e2e test to this PR, or we can have it happen in a separate PR, either way is totally fine 👍 |
If you are happy with it, I can make this change via GitHub (forgot I can do this) - if we want to push this through before RC next week. But wanted to ask permission first before messing with your PR. Let me know! |
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.
LGTM!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justjoeyuk, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 128a09cf-07ff-4d3e-8853-dd7385d2b2ea The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
What this PR does / Why we need it:
This PR fixes the issue presented in #1957 wherein the SDK returns a
nil
value for anok
value instead of analpha.Bool{ Bool: false }
. This PR ensures that the SDK always returns a value forok
and that users can expect anok
value to always be non-nil.Which issue(s) this PR fixes:
Work on #1957
Special notes for your reviewer:
This is my first contribution - so I may have made a few errors of judgement in places. I'm happy to hear feedback and make the relevant changes.