-
Notifications
You must be signed in to change notification settings - Fork 96
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
OCM-4459 | fix: change casing to fix the create failure issue #912
Conversation
UserName UsernameClaim | ||
Username UsernameClaim |
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.
You could also use the json annotation
@json(name = "username")
UserName UsernameClaim
This will ensure backward compatible for the UserName
field, assuming that the sdk has already been release.
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.
I am not aware of anyone using this yet so it should be safe. But we can also change the json annotation if we want
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. And if there were people using it, it's probably not working so we should be fine just changing it now. I wanted to share the above as mostly an FYI
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.
Thanks @machi1990 @andreadecorte , let me add the json annotation
Signed-off-by: Maggie Chen <[email protected]> add json Signed-off-by: Maggie Chen <[email protected]>
@andreadecorte @machi1990 Would you mind taking another look? 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.
I'll let @andreadecorte give this another look and final approval as the SME
/lgtm |
Hmm seems like it's not auto merged. @andreadecorte @machi1990 Do you have merge permission by any chance? |
@chenz4027 it is merged now, you can followup with the rest of the changes |
@machi1990 Thanks a lot! |
https://issues.redhat.com/browse/OCM-4459