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

adding support for hydra client metadata property #38

Merged
merged 20 commits into from
Feb 3, 2020

Conversation

amihalj
Copy link
Contributor

@amihalj amihalj commented Dec 5, 2019

  • added Metadata map[string]interface{} into OAuth2ClientSpec
  • added explicit test on POST route to check for metadata and omitted metadata

@amihalj
Copy link
Contributor Author

amihalj commented Dec 5, 2019

ok, I'm stuck, don't know what is causing it to fail. Anyone?

@aeneasr
Copy link
Member

aeneasr commented Dec 6, 2019

Looks like the generator does not like interface{} types. You could try replacing map[string]interface{} with json.RawMessage. I'm not sure how the generator works exactly but json.RawMessage simply represents a json string (e.g. {"foo":"bar"}) instead of a deserialized memory structure such as a polymorphic map!

Googling the error messages, I found the following resources:

Ands sorry for the slow response, I had tons of travel and meeting this week!

@amihalj
Copy link
Contributor Author

amihalj commented Dec 6, 2019

ok, it seems that deep copy does not like if you declare Metadata as new type json.RawMessage.

@aeneasr thank you, now it has passed all checks

Copy link

@p-bakker p-bakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some typos I believe

@@ -27,4 +27,7 @@ spec:
endpoint: /clients
forwardedProto: https
tokenEndpointAuthMethod: client_secret_basic
metadata:
property1: 1
propetry2: "2"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -37,3 +37,6 @@ spec:
endpoint: /clients
forwardedProto: https
tokenEndpointAuthMethod: client_secret_basic
metadata:
property1: 1
propetry2: "2"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it is irrelevant, since any name can be property, but I will correct it.

thanks

@aeneasr
Copy link
Member

aeneasr commented Feb 1, 2020

Sorry, I forgot to review this - could you please rebase on master and re-generate the fields? :)

@claassistantio
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ amihalj
✅ kubadz
✅ aeneasr
✅ PoulpiFr
❌ ORY Continuous Integration


ORY Continuous Integration seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@amihalj
Copy link
Contributor Author

amihalj commented Feb 3, 2020

rebased on latest master

@aeneasr aeneasr merged commit 208d145 into ory:master Feb 3, 2020
@aeneasr
Copy link
Member

aeneasr commented Feb 3, 2020

Thank you!

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.

6 participants