Skip to content
This repository has been archived by the owner on Feb 28, 2024. It is now read-only.

New CLA system #139

Merged
merged 4 commits into from
Jul 1, 2019
Merged

New CLA system #139

merged 4 commits into from
Jul 1, 2019

Conversation

OmarShehata
Copy link
Contributor

@OmarShehata OmarShehata commented Jun 27, 2019

This is an overhaul of how our CLA checking system works to fully automate the process and make it more streamlined for everyone.

Before

Concierge would check a CLA.json file hosted somewhere for each new contributor. We had to manually update this CLA.json, and separately store the actual signed CLA somewhere else.

After

Concierge now checks a Google Sheets spreadsheet, which itself contains the legal signature of the contributor. This sheet is automatically populated when the new contributor signs the Google form.

The full details of the new system are in the README https://github.com/AnalyticalGraphicsInc/cesium-concierge/blob/c93864c26029814efe5b37bda3f7c8616dcc2863/README.md#cla-checking

Reviewing Guide

@mramato I'd appreciate some high level feedback here. Specifically, if you had to maintain this moving forward, or look into it if it breaks, do you feel like you'd have everything you need?

Code suggestions are welcome too. What's left to merge and kick off this new system:

  • Test on a real, private repo, to be 100% sure it works with all the APIs before kicking into production
  • Write some more tests that it handles errors gracefully if anything goes wrong with Google Sheets API/data errors
  • Make corporate CLA check more strict? I'd rather we get a false negative (Concierge thinks someone covered by CLA is not) as opposed to false positive (concierge allows someone to contribute who isn't covered).
  • Update Concierge's instructions, to say fill out this form instead of email [email protected] Concierge just links to contributing.md, so we should update that.
  • Update CLA instructions on Cesium repo and others that concierge follows.

@OmarShehata OmarShehata requested a review from mramato June 27, 2019 15:51
@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

✨ ✨ ✨I'm the Cesium Concierge and I'm here to test things. ✨ ✨ ✨

@OmarShehata
Copy link
Contributor Author

Feedback on the process as a whole is welcome too, it's not too late to change this (why are we using Google forms etc)

@mramato
Copy link
Contributor

mramato commented Jun 28, 2019

@OmarShehata what's the easiest way for me to actually try the new process from an end-user point of view? That's the most important part

@OmarShehata
Copy link
Contributor Author

@mramato the workflow for the user is almost the same. So you would:

  1. Open the PR, concierge complains you didn't sign the CLA, with the following message:

Please fill out an Individual Contributor License Agreement (CLA) and comment back here to let us know to check this!

(Most likely instead of a direct link to form, it will be link to CONTRIBUTING.md which has link to form for individual and a form for corporate CLA)

  1. You comment back on the PR saying "I signed it!"
  2. Reviewer checks the spreadsheet (or the person watching form notifications, informs reviewer in the PR that CLA was received)
  3. Next time you open a PR, concierge says ✔️ CLA signed.

lib/deferredPromise.js Outdated Show resolved Hide resolved
return googleapis.google.auth.getClient({
keyFile: googleConfigFilePath,
scopes: ['https://www.googleapis.com/auth/spreadsheets']
}).then(function(client) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our convention these days is to use async/await for everything, will make the code much cleaner.

lib/Settings.js Outdated Show resolved Hide resolved
@OmarShehata
Copy link
Contributor Author

Thanks for all your feedback Matt! I've updated the code (except for the async/await because I think it would be a bigger clean-up change there). I'm merging this now to start the process of switching over to the new CLA checking today.

@OmarShehata OmarShehata merged commit 15548b0 into master Jul 1, 2019
@OmarShehata OmarShehata deleted the new-cla branch July 1, 2019 12:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants