search

OCA / oca-github-bot

The GitHub Bot of the Odoo Community Association (OCA)
MIT License
42 stars 60 forks source link

oca-cla-bot fails with old commits and old emails #209

Open yajo opened 2 years ago

yajo commented 2 years ago

Describe the bug

oca-cla-bot fails when somebody migrates a module that includes commits from an email that had CLA signed back then, but not anymore.

To Reproduce

See:

Expected behavior My old job email should still pass ✔️ because when I pushed those commits back then, I had the CLA signed with that email.

rousseldenis commented 2 years ago

@Yajo I think this is tied to how we do migrations.

IMHO (I'll try to open a discussion soon), migrations should include only migration commits. It's becoming more and more difficult to review migrations.

We should create new main branches from preceding one.

pedrobaeza commented 2 years ago

That procedure was a total pain, as there can be extra commits since the branch creation, and doing a periodical synchronization is also very tedious, provoking at the end also non pure migration PRs.

But you can review a PR with several commits in the current procedure, including the migration commit, very easily clicking on such commit, and GitHub shows the diff related to the migration. What is the problem with it? What is becoming more and more difficult? This procedure also serves for squashing useless administrative commits.

yajo commented 2 years ago

Well, it truly has a security problem, but outside of that I agree that the old process was even worse.

In any case, all of that is outside of the scope. The problem is that the bot complains about email addresses that were CLA-signed back then. It should compare the email and date of the commit, and then complain about those that have no CLA approval within that specific date.

pedrobaeza commented 2 years ago

Yes, I also put the same on https://github.com/OCA/oca-github-bot/issues/170. @sbidoul @gurneyalex maybe you can check it.

sbidoul commented 2 years ago

I don't think there is anything reasonable we can do to fix the current CLA bot. Long term we may want to move to a paperless and more robust process with, e.g., https://github.com/cla-assistant/cla-assistant.

pedrobaeza commented 2 years ago

@sbidoul we just need to include in the bot with_context(active_test=False) for searching inactive contacts

gurneyalex commented 2 years ago

I just added the context key in the legacy clabot. Let's see if it helps.

SirTakobi commented 1 year ago

Has https://github.com/OCA/oca-github-bot/issues/209#issuecomment-1253355953 fixed the issue? Can this be closed?