Add claims_supported to discovery info, without breaking the API #1069
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1069 +/- ##
==========================================
- Coverage 96.59% 96.58% -0.02%
==========================================
Files 32 32
Lines 1764 1787 +23
==========================================
+ Hits 1704 1726 +22
- Misses 60 61 +1 ☔ View full report in Codecov by Sentry. |
|
The Python 3.8 tests failing is probably not my fault, as I read it… |
|
I do a brief check seems okay. |
We routinely squash commits as well as rebase them so as not to clutter the project's commit history with a lot of "noise" for a single feature PR. Presumably you can track your commits in your forked repo? |
Py38 is the version used for flake8 (no point in running it for multiple different python versions). If you look at the details of that step you'll see: I recommend running tox locally before pushing. Running |
|
No, that does not work, because merges between upstream and the fork would break.
What I can offer is squashing myself, but if you want my contribution, the commit in the master branch has to carry my identity as author.
Squashing and rebasing in master is an anti-pattern though. Just don't do it. Educate PR authors to make meaningful commits instead, then keep history intact.
|
Please do squash your commits -- or are they really @AndreaGreco's?, along with fixing the flake8 error.
It's rebasing in the feature branch that we try to do; not in master. We can agree to disagree. There are religious arguments on both sides. Having a commit history for a PR that is full of noise is not helpful, especially of merges of master to get caught up vs. a clean rebase. |
|
Right. Such noise should be removed before merging. But every logical step has to remain one commit, even after merging. Merging several logical steps into one is plain wrong.
Having my commits correctly attriibuted is not religious – it is what I expect as reward for doing work free of charge. If you cannot ensure my work remains correctly attriibuted, please do not merge it.
5 Jan 2022 17:51:35 Alan Crosswell ***@***.***>:
… We can agree to disagree. There are religious arguments on both sides. Having a commit history for a PR that is full of noise is not helpful, especially of merges of master to get caught up vs. a clean rebase.
|
We are all volunteers here and I am feeling very demotivated at the moment. Thanks for that. |
|
We are all volunteers here and I am feeling very demotivated at the moment. Thanks for that.
Haha. You feel demotivated because I ask you to keep my name on my work. Sorry, but that is not my fault.
Here is what I will do: I will finish this PR with a clean commit history. After that, there are three options:
* You take it, keeping my authorship intact
* You take it, removing my authorship from the commits, and never see me again
* You don't take it, close this PR unmerged, and be happy that you turned a contributor away because they wanted their name on their work
|
|
Nobody's trying to take away credit for your or anyone else's contributions. Your contributions are valued and you are listed as the Author of commits as well as explicitly credited in the AUTHORS file with the reviewers listed as co-authors. Your threatening tone (do this and "never see me again") is unfortunate, dispiriting and demotivating for all of us. Perhaps this is just an unfortunate aspect of flaming someone you've never spoken to in person. |
That is all I am asking for. If you do ensure that what I commit under my name still is committed under my name after merging, everything is fine. However, that was not the case for the original PR by @AndreaGreco et al – two contributors lost attribution for their commits in that case, and all I am asking from you is that you do not do this when handling my commits.
The intention is not to threaten you. But we all have a base line for what we do work for, and my baseline is that I keep my commits. I do coding work for FOSS projects, I donate money to FOSS projects, and what I ask in return is to keep my commits. It is the only thing I ask for when working for FOSS, and I expect that this base line is not undercut. I don't know what it is, but I am certain such a base line exists for you as well, and that you expect others to respect it. My baseline is the above, and I ask you to respect it. No more, no less. |
|
I contribute to a couple of projects including this one and learned that it's common practice to rebase and force-push feature branches before they are merged and to squash-merge to the main branch to keep the history clean. Your concerns regarding squash-merge are news to me. Googling the issue finds strong opinions pro- and con. Meanwhile I see @AndreaGreco as the author in the commit that I unfortunately had to revert, so I'm confused. It's right there in the history, including some noise from merging master onto this PR instead of rebasing. Are we talking about the same thing? |
I see two other people listed there. I have not investigated what their original commits contained. In any case, again repeating myself: If my contribution is committed to master under my name as commit author, everything is fine. |
That's not that easy, because I do not have old Python versions available. Maybe the non-version-dependent checks should use the default python3 instead of a fixed version. In any case, trying to run flake8 manually now, and see what to fix. |
Natureshadow
force-pushed
the
claims-discovery-info
branch
from
28eba81 to
1364162
Compare
|
@n2ygk I fixed the lint errors (squashing the lint fixes into the original commits). Is the commit history now in a state in which you can merge it verbatim, or do you want me to clean up something else? |
Natureshadow
force-pushed
the
claims-discovery-info
branch
2 times, most recently
from
46122c1 to
b3c4a4e
Compare
Depending on your platform, it's fairly easy to have "all the versions" of Python with https://github.com/pyenv/pyenv I'm pretty sure that We really try to make sure all supported version of Python and Django are thoroughly tested, although you can always locally run just tox with the one Python version you have: Also https://django-oauth-toolkit.readthedocs.io/en/latest/contributing.html documents making sure to install I hope this helps. |
|
It looks like all checks are green now, or am I missing something?
What about the commit history? Is it ok to be merged like it is?
If so, a release of 1.7.0 would be much appreciated. We have a release deadline downstream, which currently needs to pin DOT down to 1.5, scheduled on Saturday.
What can I do to help make it happen?
|
|
I can't commit to that timeframe, sorry. It's not just about green checkmarks; I need to thoroughly review the code and am busy at my day job too. Last week I had a lot of free time while on vacation. I am rolling a 1.6.2 hotfix as my first priority to resolve the open issues for those who are experiencing them. |
|
I am rolling a 1.6.2 hotfix as my first priority to resolve the open issues for those who are experiencing them.
That would be entirely sufficient, thanks!
6 Jan 2022 17:38:57 Alan Crosswell ***@***.***>:
… I am rolling a 1.6.2 hotfix as my first priority to resolve the open issues for those who are experiencing them.
|
Please test with it ASAP and let me know if I got everything. |
@Natureshadow ^ sorry forgot to tag you in this message. I'm planning on pushing this today. |
|
1.6.2 looks good, thanks! |
There was a problem hiding this comment.
I've been testing this out with this demo app and have come across a small bug. The "sub" is repeated in the claims_supported even when it is not added to the dict of callables in get_additional_claims.
http://localhost:8000/o/.well-known/openid-configuration
"claims_supported": [
"sub",
"sub",
"first_name",
"last_name"
]
|
@AndreaGreco @Natureshadow Can you please take a look at #1069 (review)? I'd like to get that bug resolved before proceeding with this PR. Thanks. |
Please respond to the review requested changes and rebase ASAP as I'd like to push out 1.7.0. If you don't have time to look at the duplicate "sub" that's OK. I can fix that in a subsequent PR. It's not a huge problem. If you no longer have time or interest, let me know and I'll finish off this PR. Also, per a prior discussion in this thread, this repo is configured by the Jazzband roadie(s) to only support squash or rebase commits. That's a Jazzband standard I believe. Merge commits are not configured here, however I believe a rebase commit will record your and @AndreaGreco's authorship if you rebase carefully. Thanks. |
Some client can't use userinfo, and get propelty from claims. Add claims key inside well-known.
* always propagate request * have get_additional_claims return a dict again * allow get_additional_claims to return plain data instead of callables
This splits get_additional_claims into two forms. See documentation change for rationale.
Natureshadow
force-pushed
the
claims-discovery-info
branch
from
b3c4a4e to
7befb8b
Compare
Done.
At least one of us will only be listed in a non-standard, proprietary GitHub pseudo-header. As long as that's not me, I don't care 🤷🏼 . |
** Please do not squash the commits. I analyse my FOSS contributions automatically and really want to track commits I made.**
Description of the Change
This is a second shot at #967, after the drama with #1066 and #1068 ;).
It re-introduces the same change @AndreaGreco proposed, and in fact, it supports their exact desired API for
get_additional_claims. In addition, it supports the "traditional" API forget_additional_claimsgetting passed a request directly and producing data directly.I chose to support both due to the rationale explained in the docs in this PR.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS