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 the endpoint to the warden wrapper #138

Closed
wants to merge 1 commit into from
Closed

Add the endpoint to the warden wrapper #138

wants to merge 1 commit into from

Conversation

matteosuppo
Copy link
Contributor

Fix #137

Without the endpoint the warden client panics

Signed-off-by: Matteo Suppo <matteo.suppo@gmail.com>
@boyvinall
Copy link
Contributor

@matteosuppo Looks like you beat me to this by a few hours :) I did basically exactly the same thing, except I didn't add warden.AllowedHandlerPath. Looking at https://github.com/ory-am/hydra/blob/1e48e1819672fada95d8b24251d12cd883ba61fc/warden/warden_http.go#L72, it seems the path is basically overridden every time, so I'm not sure what the best practice is here. In any event though, your fix works for me too :)

@aeneasr
Copy link
Member

aeneasr commented Jul 13, 2016

Yeah that actually doesn't make sense - I overlooked that in review. I think you can remove all paths in the SDK and just do

        Endpoint: c.clusterURL,

@aeneasr
Copy link
Member

aeneasr commented Jul 14, 2016

could you just replace all Endpoint: pkg... with Endpoint: c.clusterURL,, test if everything works ok and push this PR? I would merge it then :)

@mfzl
Copy link
Contributor

mfzl commented Jul 14, 2016

This is an oversight on my part, I saw what @boyvinall mentioned and didn't bother adding the Endpoint.

Overlooked this part:
https://github.com/ory-am/hydra/blob/1e48e1819672fada95d8b24251d12cd883ba61fc/warden/warden_http.go#L71

AFAIK Everything else requires correct Endpoint be set.

@aeneasr
Copy link
Member

aeneasr commented Jul 14, 2016

Ah yes you're right! Thanks for chipping in!

@@ -111,7 +111,8 @@ func Connect(opts ...option) (*Client, error) {
}

c.Warden = &warden.HTTPWarden{
Client: c.http,
Endpoint: pkg.JoinURL(c.clusterURL, warden.AllowedHandlerPath),
Copy link
Member

Choose a reason for hiding this comment

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

Endpoint: c.clusterURL, :)

aeneasr pushed a commit that referenced this pull request Jul 14, 2016
@aeneasr aeneasr closed this in #142 Jul 14, 2016
aeneasr pushed a commit that referenced this pull request Jul 14, 2016
aeneasr pushed a commit that referenced this pull request Aug 5, 2016
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.

4 participants