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

Allow embedding dex in other cobra projects with custom connectors #1288

Closed
wants to merge 4 commits into from

Conversation

amdonov
Copy link

@amdonov amdonov commented Sep 5, 2018

Moved most of the code from main package to a subfolder cmd. Expose both the serve and version commands. Included a sample cmd and connector for others to use as a reference for using their own connectors.

Closes #1282

@srenatus srenatus mentioned this pull request Sep 6, 2018
@ericchiang
Copy link
Contributor

I don't think this solves #1282. You'd still need to modify the server, which we shouldn't expose.

@ericchiang
Copy link
Contributor

Also per #1020 (comment), we don't commit to the interfaces you're exposing. Anyone who does this is basically forking dex, so why not just fork it?

@amdonov
Copy link
Author

amdonov commented Sep 6, 2018

It seems as though your view is that dex is a server. I don't see why it can't be a server and a library. I'd like to use 99.9% of the code and just add a custom connector. In it's current state, I have replicate all of the code in the main package in my application to use the rest of dex's code. This minor change allows to import the code rather than copy it. If you create a new project with cobra that's how it sets it up now. main is pretty much empty.

It solves #1282 for me because I can host that connector, import, and use it without you approving it.

I didn't add it here, but you could go even further. If you move the specification of the connectors to main rather than down in server, other projects could only run with the connectors they want included. For example, if I didn't trust some connector or one of it's dependencies, I wouldn't need to import it. Right now it's all or nothing.

@ericchiang
Copy link
Contributor

Libraries require designing around APIs with compatibility in mind. We change stuff all the time, plumb new types through dex, and users shouldn't depend on these internal packages. One day we may split out the OAuth2 code into something like https://github.com/RangelReale/osin, but until then I don't want users importing the dex server or depending on the connector interfaces being stable. That's not what it's intended for and would place a huge burden on the dex maintainers.

Closing to save time going back and forth. I'm fine refactoring code, but not having other projects depend on dex's internal types.

Please fork dex if you need to support connectors we don't want to merge into main, or consider chiming in on some of our other efforts to support more generic user backends #1020

@ericchiang ericchiang closed this Sep 6, 2018
@amdonov
Copy link
Author

amdonov commented Sep 6, 2018

I understand, but I wish you felt differently. You're not signing a support contract with folks.

Of course, you're free to change your API. That's what versions are for. :-)

Looking forward to seeing #1020. Not likely to commit anything. Merge approval is too opinionated for me.

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.

2 participants