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

Require the connector to have an ID. #686

Merged
merged 1 commit into from
Nov 15, 2016
Merged

Require the connector to have an ID. #686

merged 1 commit into from
Nov 15, 2016

Conversation

cjyar
Copy link

@cjyar cjyar commented Nov 15, 2016

No description provided.

@ericchiang
Copy link
Contributor

cmd/dex/serve.go:116: invalid operation: conn.ID == nil (mismatched types string and nil)

@ericchiang
Copy link
Contributor

Thanks for the PR! ID is a string so you'll have to do conn.ID == ""

Updates #681

@cjyar
Copy link
Author

cjyar commented Nov 15, 2016

D'oh! I guess it helps to make sure the code compiles before I send a PR.

@ericchiang
Copy link
Contributor

@cjyar that's why we have CI :)

@ericchiang
Copy link
Contributor

@cjyar can you squash and edit the commit message to begin with cmd/dex. That's just a CoreOS style thing. For example

cmd/dex: validate that connectors have an ID

@cjyar
Copy link
Author

cjyar commented Nov 15, 2016

@ericchiang It won't let me. I can squash them in my repo, delete this PR, and make a new one. Or you can squash them on your end with the pulldown on the "Merge pull request" button. That button doesn't show up for me, because I don't have permission to merge into your repo.

@SEJeff
Copy link
Contributor

SEJeff commented Nov 15, 2016

@cjyar You can squash them locally on your workstation and force push to the master branch on your fork under @cjyar. That will then update this PR with a squashed version with the requested changes.

@cjyar
Copy link
Author

cjyar commented Nov 15, 2016

@SEJeff Thanks for the tip!

@ericchiang
Copy link
Contributor

lgtm!

@ericchiang ericchiang merged commit 2ec3349 into dexidp:master Nov 15, 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.

3 participants