-
Notifications
You must be signed in to change notification settings - Fork 327
Update contribution guidelines #1885
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
Conversation
DrJosh9000
left a comment
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.
Here, have these minor capitalisation comments!
5309b86 to
52cef12
Compare
52cef12 to
5388cd0
Compare
DrJosh9000
left a comment
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.
One nit but LGTM 😎
I think there's maybe an argument for moving it to a CONTRIBUTING.md, but this is good!
triarius
left a comment
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.
As we discussed in triage, @mitchbne and I think it would be a good idea to make a new file for visibility. We can link to it from here, so I don't think it would make it that much less visible. It's potentially more visible as the files are shown above the README.md render in the GitHub UI. Less scrolling is required to see the file than the heading in the markdown.
README.md
Outdated
| 1. Push to your the branch (`git push origin my-new-feature`) | ||
| 1. Create new Pull Request | ||
|
|
||
| The agent wranglers at Buildkite will review your PR, and start a CI build for it. For security reasons, we don't automatically run CI against forked repos, and a human will review your PR prior to its CI running. |
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.
agent wranglers
Nice! I look forward to us putting this on our business cards 🪪
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'm looking forward to getting the business cards in the first place :P
README.md
Outdated
|
|
||
| The agent wranglers at Buildkite will review your PR, and start a CI build for it. For security reasons, we don't automatically run CI against forked repos, and a human will review your PR prior to its CI running. | ||
|
|
||
| Our objective is to have no PR wait more than a week for some sort of interaction from us -- this might be a review, or it might be a "I'm going to come back to this and review it a bit later". If we're really dragging our feet on reviewing a PR, please feel free to ping us through GitHub or Slack, or get in touch with support@buildkite.com, and they can bug us to get things done :) |
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.
objective is to have no PR wait more than a week
I'm quite hesitant to commit to this publicly before we start measuring it internally.
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.
yeah, that's a good point. i've added a sentence in there about how it's a goal and not a guarantee, rather than omitting the timeline stuff completely, mostly because it's tricky to find a way of saying "we'll review your PR quickly or maybe we won't" in a nice way.
e214f6a to
f611de1
Compare
triarius
left a comment
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!

this is super rough and probably needs a coat of ✨developer relations✨ on it, but would love to have some thoughts!