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

Openid Connect Core support - Round 2 #859

Merged
merged 57 commits into from
Sep 6, 2020

Conversation

thinkwelltwd
Copy link
Contributor

I would like to use DOT, but need oidc support so I rebased shaun's PR onto wiliamsouza's PR and then rebased that onto master.

If it's not kosher to open another PR with other people's work, please let me know and I'll delete it. Otherwise, I would be very glad to get oidc support in this library.

wiliamsouza and others added 30 commits August 28, 2020 15:46
…n_required to be 400 as changed in oauthlib/pull/623
@coveralls
Copy link

coveralls commented Aug 28, 2020

Pull Request Test Coverage Report for Build 1423

  • 234 of 274 (85.4%) changed or added relevant lines in 11 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.0%) to 93.807%

Changes Missing Coverage Covered Lines Changed/Added Lines %
oauth2_provider/views/mixins.py 11 13 84.62%
oauth2_provider/oauth2_backends.py 4 8 50.0%
oauth2_provider/views/oidc.py 42 47 89.36%
oauth2_provider/oauth2_validators.py 122 134 91.04%
oauth2_provider/models.py 31 48 64.58%
Files with Coverage Reduction New Missed Lines %
oauth2_provider/oauth2_validators.py 2 90.8%
Totals Coverage Status
Change from base Build 1416: -1.0%
Covered Lines: 1439
Relevant Lines: 1534

💛 - Coveralls

tox.ini Outdated Show resolved Hide resolved
@auvipy auvipy merged commit 4655c03 into jazzband:master Sep 6, 2020
@thinkwelltwd thinkwelltwd deleted the openid-connect-2 branch September 7, 2020 11:39
@pawalt
Copy link

pawalt commented Sep 7, 2020

Hey there! Thanks for this PR; my team is very excited about getting started with it. Are there any examples or docs we could follow to get started?

Happy to move to an issue as well if comments on closed PRs aren't permitted.

@n2ygk
Copy link
Member

n2ygk commented Sep 21, 2020

@auvipy you've merged a significant bunch of changes but didn't request that the checklist be followed and @thinkwelltwd I'm wondering why this checklist is not in the PR description as it must have been templated there. Did you (inadvertently) remove it?:

- [ ] PR only contains one change (considered splitting up PR)
- [ ] unit-test added
- [ ] documentation updated
- [ ] `CHANGELOG.md` updated (only for user relevant changes)
- [ ] author name in `AUTHORS`

Specifically I see a number of problems that will require a follow-up PR @thinkwelltwd:

  • PR only contains one change (considered splitting up PR)
    I guess it's too late to confirm if this is the case.
  • unit-test added
    I see a bunch of tests are added, and coverage has decreased a little bit, so hopefully the added tests are enough.
  • documentation updated
    I see no documentation of the new OIDC features. Please document. All this great work is worth nothing to other users if they can't see that it's there. We don't merge code just for personal use.
  • CHANGELOG.md updated (only for user relevant changes)
    This is major new functionality, requiring a major release per semver yet there's nothing about it in the CHANGELOG.
  • author name in AUTHORS
    @thinkwelltwd please add your name. Credit where credit is due along with those of @shauns and @wiliamsouza

Would @thinkwelltwd and/or @auvipy please submit a documentation PR to remediate this issue? I'm reluctant to push a new release with the master branch in the state it is right now.

@auvipy I'm kind of disappointed as we've discussed this issue of merging before ready before. This lack of process would someone considering using this library to think twice about the quality of it.

@n2ygk n2ygk mentioned this pull request Oct 1, 2020
2 tasks
n2ygk added a commit that referenced this pull request Oct 6, 2020
n2ygk added a commit that referenced this pull request Oct 6, 2020
@n2ygk
Copy link
Member

n2ygk commented Oct 6, 2020

This will need to be re-opened/recreated as a new PR.

@tevansuk tevansuk mentioned this pull request Oct 12, 2020
5 tasks
@n2ygk n2ygk mentioned this pull request Oct 14, 2020
@tevansuk tevansuk mentioned this pull request Jan 12, 2021
10 tasks
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.

10 participants