Skip to content

[CI] Check contributor user ID #6578

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

Merged
merged 3 commits into from
Jan 28, 2021
Merged

Conversation

ppenzin
Copy link
Member

@ppenzin ppenzin commented Jan 15, 2021

Check if a user ID of the submitter is in the Contributor Agreement file.

@ppenzin ppenzin force-pushed the azure-cla-check branch 2 times, most recently from d2d5435 to 57bfe5b Compare January 15, 2021 02:22
@ppenzin
Copy link
Member Author

ppenzin commented Jan 15, 2021

The job is failed with an obscure message about short-circuiting, brief search showing that it might be some sort of Pipelines glitch.

@ppenzin ppenzin force-pushed the azure-cla-check branch 2 times, most recently from 5baad76 to 339f17e Compare January 15, 2021 02:44
@ppenzin ppenzin marked this pull request as draft January 15, 2021 02:54
@ppenzin
Copy link
Member Author

ppenzin commented Jan 15, 2021

Alright, this approach (trying to get user ID of the submitter) might not work - it does not get the user name in Build.RequestedFor and Build.RequestedForId. We might need to either finish set up with claassistant or find some other way to read the user name.

@ppenzin
Copy link
Member Author

ppenzin commented Jan 15, 2021

Also, it looks like a variation of the tool we are using might be able to check for signatures in the file as well as agreement in the repo: https://github.com/cla-assistant/github-action

@ppenzin
Copy link
Member Author

ppenzin commented Jan 16, 2021

@rhuanjl @Fly-Style what do you think about continuing on this path vs the license/cla checker? To keep "our own" checker need a better strategy of getting Github handles - I've looked in Azure docs, and it does not seem like they provide something like that (despite UI clearly knowing who triggered the build). On the other hand cla-assistant should be able to do it via Github actions.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 16, 2021

My preference is this approach if we can do it - if the github actions API lets us grab commit user names could we do it with a minimal script like this PR but github actions instead of Azure?

@ppenzin
Copy link
Member Author

ppenzin commented Jan 23, 2021

I don't really see anything that would easily provide the username, the best bet is using the API, which is done in full version of CLA check.

Test repo using this approach: https://github.com/ppenzin/cla-sign-check

We can customize what the bot says, how things are called and where they are stored.

@ppenzin
Copy link
Member Author

ppenzin commented Jan 23, 2021

@rhuanjl you can open a PR in the test repo to see how bot would interact with you: https://github.com/ppenzin/cla-sign-check

End result of my interaction with it: ppenzin/cla-sign-check#2

The only downside is that it edits its own message instead of adding a new one when the agreement is signed, but maybe there is a way to change that.

Signatures are stored in signatures/version1, and settings are in .github/workflows. We can change the wording of the messages, names of the files, etc.

@ppenzin ppenzin marked this pull request as ready for review January 26, 2021 03:05
@ppenzin
Copy link
Member Author

ppenzin commented Jan 26, 2021

@rhuanjl turns out Actions support querying usernames. This should work, though I am not sure if this PR is going to get checked by it. I did prepare the other variant as well.

POC is in https://github.com/ppenzin/cla-grep-check (note you are already in the file, as I just copied it from here).

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 26, 2021

This is really nice - I like how totally lightweight it is. Not as many features as some of the other options but does what it needs.

(One thing I'd perhaps change is deleting the space out of the name "Contribution Agreement.md" I know I put it there in the first place but it causes problems with the style check scripts and it's currently the only file in our source tree with a space in the name (I think). We'd have to change it in a couple of other places too Readme.md and Contributing.md both ref it.)

@Fly-Style
Copy link
Contributor

Looks good to me. Probably, I'd disable cla-assistant.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 26, 2021

Obviously we've got options here and need to pick one - I think we've looked at 3:

  1. this one - automated checking of the list in the agreement file
  2. https://github.com/ppenzin/cla-sign-check bot that checks, stores signatures from comments, does it all on file but uses a json doc to store the data
  3. Create automated CLA checking #6575 bot with an interface to click through and sign and storage off file

Any of the 3 work:
(1) and (2) both have the benefit of all being on file so nothing outside of the repository that needs maintaining.
(1) avoids any complex machinery
(2) has the nice point of new contributors signing via a comment.
(3) may look more professional with it's click through interface
(1) has the downside of requiring contributors to submit a commit to sign the agreement

My preference is (1) but I could see us using any of them so - which one is your favourite @ppenzin @Fly-Style ?

Check if a user ID of the submitter is in the Contributor Agreement file.
@ppenzin
Copy link
Member Author

ppenzin commented Jan 27, 2021

Squashed two commits together (nobody really needs to see broken Pipelines solution) and dropped space from the agreement file. We can also wire the rest of CI to fire only after CLA check, to skip building PRs we can't merge for CLA reasons.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 28, 2021

@Fly-Style please do drop cla-assistant, thanks for looking into it as an option though.

@rhuanjl rhuanjl merged commit 0024ebf into chakra-core:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants