Skip to content

Conversation

@wolfeidau
Copy link
Contributor

No description provided.

@gopherbot
Copy link
Contributor

This PR (HEAD: 388eb20) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/oauth2/+/210746 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1:

See https://github.com/golang/oauth2/#policy-for-new-packages

We've stopped accepting new packages as it kinda got out of control.

Perhaps we just need one new package "endpoints" with a bunch of these in it.


Please don’t reply on this GitHub thread. Visit golang.org/cl/210746.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mark Wolfe:

Patch Set 1:

Patch Set 1:

See https://github.com/golang/oauth2/#policy-for-new-packages

We've stopped accepting new packages as it kinda got out of control.

Perhaps we just need one new package "endpoints" with a bunch of these in it.

I did a bit of quick analysis and there are:

  • 34 service providers with hard coded URLs eg. Slack or GitHub

  • 3 service provider has two sets of hard coded URLs in the one package, which follow the convention of having declared Endpoint as well as and SomethingEndpoint.

  • 3 service providers have a function which build the endpoint URLs, being jira, hipchat server and microsoft AzureAD. Most of these already have an existing hard coded endpoint.

I had a look over the URLs and there isn't a lot of consistency in them so there aren't a lot of options for building a subset based on a common path.

As you suggested it would be possible to construct a new package with a lookup table of service provider to auth/token URL, possibly with the option of having a sub tag for the few cases with one or more endpoints.

For the dynamic endpoints these could be handled separately as there are only a few, typically for multi tenant services or those with configurable domains such as the AWS Cognito one I submitted.

Do you know of any other instances of this sort of problem in an existing go code base?

Could a solution be based off of one of them?

Cheers


Please don’t reply on this GitHub thread. Visit golang.org/cl/210746.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1:

Patch Set 1:

Patch Set 1:

See https://github.com/golang/oauth2/#policy-for-new-packages

We've stopped accepting new packages as it kinda got out of control.

Perhaps we just need one new package "endpoints" with a bunch of these in it.

I did a bit of quick analysis and there are:

  • 34 service providers with hard coded URLs eg. Slack or GitHub

  • 3 service provider has two sets of hard coded URLs in the one package, which follow the convention of having declared Endpoint as well as and SomethingEndpoint.

  • 3 service providers have a function which build the endpoint URLs, being jira, hipchat server and microsoft AzureAD. Most of these already have an existing hard coded endpoint.

I had a look over the URLs and there isn't a lot of consistency in them so there aren't a lot of options for building a subset based on a common path.

As you suggested it would be possible to construct a new package with a lookup table of service provider to auth/token URL, possibly with the option of having a sub tag for the few cases with one or more endpoints.

For the dynamic endpoints these could be handled separately as there are only a few, typically for multi tenant services or those with configurable domains such as the AWS Cognito one I submitted.

Do you know of any other instances of this sort of problem in an existing go code base?

Could a solution be based off of one of them?

Cheers

I was just thinking a new package named "endpoints" that literally lists a bunch of endpoints. No maps, no fanciness:

package endpoints

var Twitter = oauth2.Endpoint{....}

var AWSCognito = oauth2.Endpoint{....}

etc.


Please don’t reply on this GitHub thread. Visit golang.org/cl/210746.
After addressing review feedback, remember to publish your drafts!

wolfeidau added a commit to wolfeidau/oauth2 that referenced this pull request Dec 20, 2019
As per discussion in golang#401 and gerrit I have built out the proposed endpoint package.

I migrated all the existing endpoints, not sure if you wanted this but it does illustrate the pattern.
wolfeidau added a commit to wolfeidau/oauth2 that referenced this pull request Dec 20, 2019
As per discussion in golang#401 and gerrit I have built out the proposed endpoint package.

I migrated all the existing endpoints, not sure if you wanted this but it does illustrate the pattern.
@gopherbot
Copy link
Contributor

Message from Mark Wolfe:

Patch Set 1:

Patch Set 1:

Patch Set 1:

Patch Set 1:

See https://github.com/golang/oauth2/#policy-for-new-packages

We've stopped accepting new packages as it kinda got out of control.

Perhaps we just need one new package "endpoints" with a bunch of these in it.

I did a bit of quick analysis and there are:

  • 34 service providers with hard coded URLs eg. Slack or GitHub

  • 3 service provider has two sets of hard coded URLs in the one package, which follow the convention of having declared Endpoint as well as and SomethingEndpoint.

  • 3 service providers have a function which build the endpoint URLs, being jira, hipchat server and microsoft AzureAD. Most of these already have an existing hard coded endpoint.

I had a look over the URLs and there isn't a lot of consistency in them so there aren't a lot of options for building a subset based on a common path.

As you suggested it would be possible to construct a new package with a lookup table of service provider to auth/token URL, possibly with the option of having a sub tag for the few cases with one or more endpoints.

For the dynamic endpoints these could be handled separately as there are only a few, typically for multi tenant services or those with configurable domains such as the AWS Cognito one I submitted.

Do you know of any other instances of this sort of problem in an existing go code base?

Could a solution be based off of one of them?

Cheers

I was just thinking a new package named "endpoints" that literally lists a bunch of endpoints. No maps, no fanciness:

package endpoints

var Twitter = oauth2.Endpoint{....}

var AWSCognito = oauth2.Endpoint{....}

etc.

Gday Brad,

I had a shot at doing this in https://go-review.googlesource.com/c/oauth2/+/212223/1 would love some feedback.

Cheers.


Please don’t reply on this GitHub thread. Visit golang.org/cl/210746.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/210746 has been abandoned.

Closing in favor of https://go-review.googlesource.com/c/oauth2/+/212223

@gopherbot gopherbot closed this Jan 3, 2020
gopherbot pushed a commit that referenced this pull request Jan 7, 2020
As per discussion in #401 and gerrit I have built out the proposed endpoint package.

I migrated all the existing endpoints, not sure if you wanted this but it does illustrate the pattern.

Change-Id: I53f56a06207633b2380b7cd7332cd56f9ef6578f
GitHub-Last-Rev: fde9e7b
GitHub-Pull-Request: #402
Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/212223
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants