search

wandersoncferreira / code-review

Code Reviews in Emacs
https://wandersoncferreira.github.io/code-review/
GNU General Public License v3.0
462 stars 49 forks source link

Use API passwords configured for forge #212

Closed hjudt closed 2 years ago

hjudt commented 2 years ago

code-review searches for authentication data in ~/.authinfo.gpg by looking for logins ending with "^code-review", similar to how magit forge looks for logins ending with "^forge". Since code-review depends on forge and many users may already have configured magit forge, use the forge passwords. This way, the user does not have to add more lines to the authinfo.gpg file.

This fixes issue #211.

wandersoncferreira commented 2 years ago

Amazing @hjudt .

I have a question related to backwards compatibility, we should try to minimize breaking changes as much as possible. I think it's possible for users to have configured specific tokens for code-review providing more grants than the ones used in forge.

Could you check if my assumption is correct? Do both tokens always requires the same grants ? A good enough check would be going to both documentation of forge and code-review and verifying which grants are recommended to users.

How do you suggest to minimize this breaking changes to users? should we support both 'forge and 'code-review for a while? should we try some /whoami endpoint with both credentials and prefer 'forge if both successful?

wandersoncferreira commented 2 years ago

Very minor, but please add an entry in the changelog and contributors file.

hjudt commented 2 years ago

I have only used forge with gitlab so far, and there is only an api scope that grants pretty everything. BTW: code-review doesn't really work with gitlab atm, there seems to be more functionality in forge than in code-review, like setting assignees etc.

I would be fine with a custom variable to choose between 'forge, 'code-review or something else. This was my first solution, though I thought it would be better to minimize the number of options one needs to care about. I will push another PR so you can decide.

hjudt commented 2 years ago

See https://github.com/wandersoncferreira/code-review/pull/213

wandersoncferreira commented 2 years ago

Ok, let's merge #213 and I'll be running with 'forge for a while and then we decide how to make the complete flip. Could you add a section in the REAME called Experimental to document this new option? I'll be closing this one.