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

Add wrapper library for HTTP Managers #130

Merged
merged 17 commits into from
Jul 3, 2016
Merged

Add wrapper library for HTTP Managers #130

merged 17 commits into from
Jul 3, 2016

Conversation

mfzl
Copy link
Contributor

@mfzl mfzl commented Jun 30, 2016

No description provided.

@aeneasr
Copy link
Member

aeneasr commented Jun 30, 2016

How about moving the package to ory-am/hydra/sdk?

@aeneasr
Copy link
Member

aeneasr commented Jun 30, 2016

Could you please add unit tests and sign the DCO using git commit -s

@aeneasr aeneasr added feat New feature or request. documentation labels Jun 30, 2016
@aeneasr
Copy link
Member

aeneasr commented Jun 30, 2016

Oh and a note in the docs would be great! Maybe in the FAQ section? :)

@mfzl
Copy link
Contributor Author

mfzl commented Jun 30, 2016

Can't build now, I'm getting this:

pkg/test_helpers.go:65: unknown strategy.HMACSHAStrategy field 'AccessTokenLifespan' in struct literal
pkg/test_helpers.go:66: unknown strategy.HMACSHAStrategy field 'AuthorizeCodeLifespan' in struct literal

Signed-off-by: Mohamedh Fazal <mohamedhfazal@gmail.com>
@aeneasr
Copy link
Member

aeneasr commented Jun 30, 2016

make sure to update your local dependencies or use glide, fosite got updated last night

mfzl and others added 3 commits June 30, 2016 15:58
Signed-off-by: Mohamedh Fazal <mohamedhfazal@gmail.com>
Signed-off-by: Mohamedh Fazal <mohamedhfazal@gmail.com>
```
2. Use the API

**OAuth Clients**: `client.Client`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add examples here? e.g.

// client, err := ...
oauth2Client, err := client.Client.GetClient("1234")

@mfzl
Copy link
Contributor Author

mfzl commented Jul 2, 2016

When you said unit tests, did you mean something similar to tests in HTTP Managers or did you have something else in mind?

@aeneasr
Copy link
Member

aeneasr commented Jul 2, 2016

You don't need to test the embedded structs (e.g. ClientManager), those are already well tested. It is enough to test new methods such as FromYAML(), ClientID(), ...

@mfzl
Copy link
Contributor Author

mfzl commented Jul 2, 2016

I forget to sign every other commit and it looks messy. Sorry about that, is that okay?. Shall I create a new PR?

Signed-off-by: Mohamedh Fazal <mohamedhfazal@gmail.com>
}

// SkipSSL skips TLS verification
func SkipSSL() option {
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename this to SkipTLSVerify, it's clearer then that the tls verification is disabled, not SSL in total

@aeneasr
Copy link
Member

aeneasr commented Jul 3, 2016

no that's fine, you don't need to sign every commit. I will squash them anyways :) thanks for your hard work!

Signed-off-by: Mohamedh Fazal <mohamedhfazal@gmail.com>
@aeneasr
Copy link
Member

aeneasr commented Jul 3, 2016

thanks!

@aeneasr aeneasr merged commit 266b324 into ory:master Jul 3, 2016
@mfzl
Copy link
Contributor Author

mfzl commented Jul 3, 2016

Thank you! For being patient with me. 😃

@aeneasr
Copy link
Member

aeneasr commented Jul 3, 2016

No, thank you for your contribution 👍 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants