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

The CLA checker doesn't handle multiline From: #29

Open
levitte opened this issue Mar 14, 2024 · 2 comments
Open

The CLA checker doesn't handle multiline From: #29

levitte opened this issue Mar 14, 2024 · 2 comments

Comments

@levitte
Copy link
Member

levitte commented Mar 14, 2024

It was discovered that the CLA checker "succeeded" falsely with openssl/openssl#23632

The CLA checker finds the author id by fetching the patch url (in this case, https://github.com/openssl/openssl/pull/23632.patch) and matching group one from From:.*<(.*)>.

This had me take a close look at the patch url, and found this:

$ curl -k -L -i https://github.com/openssl/openssl/pull/23632.patch
...
From: =?UTF-8?q?Viliam=20Lej=C4=8D=C3=ADk?=
 <34658965+lejcik@users.noreply.github.com>
...

That's a two-line From:, which our cla checker is not at all prepared for. And unfortunately, the logic in the CLA checker is that if a (one-line) From: line hasn't been found, the CLA checker will happily say that a CLA has been found.

This isn't enormously problematic, because a later run of addrev will discover that there is no CLA, and thereby fail to perform. However, this is a bit confusing.

@quarckster
Copy link
Member

Do you think it makes sense to parse the webhook payload and get commit data from it?

@levitte
Copy link
Member Author

levitte commented Mar 14, 2024

Do you think it makes sense to parse the webhook payload and get commit data from it?

It does parse the webhook payload to find the patch URL. However, the CLA checker must look at the contents of the patch URL for the following reasons:

  • The patch URL may contain more than one patch (one for each commit in the PR)
  • Each patch may have a different author, and they must all be checked against the CLA DB
  • The patches have the git author ID (as a From: field), which is what we match against the CLA DB. The webhook payload doesn't have this information.

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

No branches or pull requests

2 participants