-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
chore: add contributors #4756
chore: add contributors #4756
Conversation
1e9f951
to
7490a15
Compare
@achrinza, i'm not familiar with |
@dhmlau There's a CLI that checks for contributors that haven't been added yet. I took the output of that and ran the "all-contributors add" command for all those contributors (excluding project architects, maintainers and bots). Right now it's somewhat manual-ish. But it should be possible to automate this with a JS script. I'm not sure if Travis-CI can run cron-like jobs. |
7c1a0f0
to
e4cb3af
Compare
@raymondfeng Good idea, I've added it in. |
README.md
Outdated
<!-- markdownlint-enable --> | ||
<!-- prettier-ignore-end --> | ||
|
||
<!-- ALL-CONTRIBUTORS-LIST:END --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the code generated by all-contributors generate
doesn't play well with Prettier (The extra newline on L188 is not there when generated). So I've added an extra step to the npm script to run Prettier against README.md
.
I'm not sure if this is a good way to do it.. I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @achrinza for this proposal.
I don't have a strong opinion on this topic. Personally, I don't mind the current README, but I also like the newly proposed one.
I guess my only concern is that we will have to remember to periodically run the new npm script and commit the changes.
For longer term (but out of scope of this pull request), it would be great to find a way how to automate these updates. Perhaps via GitHub Actions?
I also found https://github.com/all-contributors/all-contributors-bot, but I am not sure if that will automatically add new contributors when their first PR is landed.
I think that's a great idea; it can be a stepping-stone to testing out GitHub actions for issues such as #3161.
AFAIK it's not automated; the bot will still need to be called via a PR comment. |
180692a
to
fa41612
Compare
+1 for adding the contributor list section. Thanks @achrinza for looking into this. It would be good to looking at GitHub Actions for updating the contributor list automatically soon, even if it's not a pre-requisite for landing the PR. |
|
@emonddr, we can run cron jobs on travis. In loopback.io, we ran some cron job daily as well. But i'm not sure where to specify which scripts to run in that cron job. If you can find out, I can change it too (if you don't have the rights on Travis to do that). |
Travis cron jobs can be set at https://travis-ci.com/github/strongloop/loopback-next/settings |
@raymondfeng, do you know how to specify what scripts to run? |
It probably runs what's defined in The other option is to have a task based on the following env var:
|
I think it might be easier to do this with GitHub Actions than with Travis. You could use something like Update File on GitHub to update the file directly or Create or Update Pull Request to submit the change as a pull request for review. Either action could be configured to be triggered by pushing or on a cron schedule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as the first step in a possible longer journey 👍
@achrinza can you please resolve the merge conflicts in package & package-lock files? It may be easiest to revert the changes in those two files, run git rebase master
and then npm install
. Just an idea 🤷♂️
Lets allow few more days for @dhmlau @raymondfeng @emonddr to raise any objections they may have. If there are none, then I think we can proceed to land this PR 👍
BTW I think it would be great to some docs for updating the list of maintainers, perhaps to docs/MAINTAINING.md? Describe the npm
scripts that are available, show examples on how to record non-code contributions, etc. This can be done in a follow-up pull request if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to land this PR and have follow up task on automating this problem and documenting it. Thanks @achrinza.
76b59af
to
7738292
Compare
1e6209f
to
ba35e3f
Compare
docs/MAINTAINING.md
Outdated
|
||
2. Push the changes. | ||
|
||
Run `git push` to push the new commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe our master
branch is protected and may not allow direct pushes. I think it's better to open a pull request even for these semi-automated changes. Not only it give us a chance to run CI and review the changes, it will also trigger a notification to people watching the project. (Commits don't notify.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos @raymondfeng The instructions were assuming that the readers just opened a PR, and that the workflow would be:
- User opens a PR for a fix/feat/chore/etc.
- User runs
contributors:add
which adds another commit - User pushes the new commit
- PR gets merged, with both the original and the attribution commit.
The idea was to minimize the number of PRs and the amount of effort. It also assums that the user would've checked out a new branch if the PR was only to add attribution, as we already have a detailed page on how to contribute to LoopBack 4 which documents this process.
If we split it into 2 separate PRs, the workflow would become:
- User opens a PR for a fix/feat/chore/etc. (original PR)
- Original PR gets merged
- User checks out a new branch for adding attribution
- User runs
contributors:add
which creates a new commit - User pushes the new commit and branch
- User creates PR (attribution PR)
- Attribution PR gets merged
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions in this section don't mention pull requests at all, that's why I assumed the intention is to commit directly to master.
- Add an attribution
- Push the changes.
Can you please add more content to this section and clarify that we expect a feature branch & a pull request to be used? I think we don't need to duplicate instructions for working with feature branches and pull requests (we already have docs for that elsewhere), just mention it in the list of steps.
I am not sure if it's a good idea to ask contributors to add themselves to the list as part of their pull request contributing a feature. It adds noise to pull requests and increases chances of merge conflicts. I think it's better to update contributors on a semi-regular basis using a short-lived pull request (i.e. create a new branch, update contributors, open a new PR, wait for CI to finish, merge the PR without waiting for review).
Maybe we can support both options and explain in the documentation why/when to use which one?
@achrinza Would it be even better to automatically update |
@raymondfeng Sounds like a good idea. I'm still working out what's the best way to go about it. The all-contributors CLI has a Node.js API, but it's experimental. Nonetheless, I'm trying to see how we can utilize the CLI where practical. |
We just switch the contribution method from CLA to DCO, making your contribution easier in the future. Please sign the commits with DCO by amending your commit messages with
Please refer to this docs page for details. Thanks! |
ba35e3f
to
6de8a5e
Compare
Signed-off-by: Rifa Achrinza <[email protected]>
6de8a5e
to
49207c6
Compare
This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity. |
This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one. |
This is a draft PR to demonstrate a POC on using the all-contributors spec.fixes #981
Signed-off-by: Rifa Achrinza [email protected]
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈