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

fix(option): skip nil bytes for CredentialsJSON #2643

Closed
wants to merge 1 commit into from

Conversation

lexcao
Copy link

@lexcao lexcao commented Jun 19, 2024

Closes #2647
Nil bytes was treated to empty bytes, which causes failure on empty JSON.
I think we can improve this by ignoring nil bytes.

Nil bytes was treated to empty bytes, which causes failure on empty JSON.
I think we can improve this by ignoring nil bytes.

Signed-off-by: Lex Cao <[email protected]>
@lexcao lexcao requested a review from a team as a code owner June 19, 2024 15:49
Copy link

google-cla bot commented Jun 19, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@quartzmo
Copy link
Member

@lexcao Please create a bug report (or possibly, feature request, depending on how you view this), we need that before we can move forward with this PR.

@quartzmo quartzmo added the needs more info This issue needs more information from the customer to proceed. label Jun 20, 2024
@quartzmo
Copy link
Member

In your issue, please explain why you think adding the conditional to avoid setting the nil value needs to be in this package, and not just in the user code such as in your PR authzed/spicedb#1948.

@lexcao
Copy link
Author

lexcao commented Jun 21, 2024

Hi @quartzmo
Thanks for reviewing, let me create an issue to provide more context first!

@lexcao
Copy link
Author

lexcao commented Jun 21, 2024

Hi @quartzmo
I have created an issue #2647.
Please help review.
Thanks!

@codyoss
Copy link
Member

codyoss commented Jul 2, 2024

I am going to close this as I opened up an alternative PR, thank you for the report though!

@codyoss codyoss closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info This issue needs more information from the customer to proceed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow skipping nil bytes for WithCredentialsJSON
3 participants