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

docs: add instructions and details to contributors guide #105

Merged
merged 2 commits into from
May 12, 2020

Conversation

lance
Copy link
Member

@lance lance commented Apr 30, 2020

  • Added detailed instructions for outside contributors.
  • Added tips for maintainers
  • Documented branching strategy

Fixes: #85
Fixes: #72

Signed-off-by: Lance Ball lball@redhat.com

@lance lance force-pushed the 72-contributors-guide branch 2 times, most recently from 13f2c0c to 3dbb6a3 Compare April 30, 2020 20:42
@lance
Copy link
Member Author

lance commented Apr 30, 2020

By way of example, I have submitted a pull request for backporting on the v1.x.y branch. This pull request is a cherry pick of the commit where the fix landed on master. In the commit message I've included a reference to this and the PR that the fix was proposed in. This allows us to cut a v1.1.0 bug fix release from the v1.x.y branch without having to worry about potential breaking changes on master.

I've also, inadvertently, provided examples of force pushing minor changes to my fork. lol

Currently our CI is configured to build all branches, but ultimately I think it should only be building master, these version branches, and pull requests.

grant
grant previously requested changes May 1, 2020
Copy link
Member

@grant grant left a comment

Choose a reason for hiding this comment

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

I suggest we simplify this contributing file and link to other standard contributing guides.

Other very popular and successful CE SDKs do not have a contributing guide as far as I see. I would imagine most people don't want to read a custom guide.

Perhaps raise this issue in the weekly CE SDK call and get consensus.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@lance
Copy link
Member Author

lance commented May 6, 2020

So I raised this in the #cloudeventssdk Slack channel and got zero engagement. The fact that the Go SDK (or any of the others) don't have contributor's guides doesn't mean in my mind that it's OK not to. It's pretty standard practice, and if I were contributing to those repos, I would make the same suggestion.

Do you have some suggested contribution guides that you think we should link to?

Are there specific concerns you have with my proposed workflow suggestions, other than "people don't want to read that"? Is it just the git workflow that you have problems with? Or is it the whole thing? Could you be specific about the sections you object to? I'm happy to accept feedback, but I'm not sure what you mean by simplifying.

I loosely based the workflow portion of this on the Node.js contributor's guide, which is actually much more detailed and longer than what I have written. Of course I realize that it's a much bigger project as well, so our processes don't need to be as strict. But don't you think it would be nice to have a standard set of recommendations for contributors to follow? It's not like I'm saying nothing will land unless you follow these steps. It just makes it easy to point to an existing doc when people have questions about how to go about it. And it makes the commit log appear clean and consistent.

I want to keep the barriers to contribution low. I want first time contributors to feel confident in their work, so that it's less scary to submit a PR to a new org. I think providing more - not less - information, tips, guidance is a way to achieve that.

@lance lance self-assigned this May 6, 2020
@lance lance added type/discussion Issues that need to be decided/debated/discussed module/docs Module documentation changes labels May 6, 2020
@slinkydeveloper slinkydeveloper self-requested a review May 7, 2020 11:48
Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

I love this initiative, as soon as is merged i think all our SDKs should adopt this. sdk-rust has a small guide that just explains the basis of configuring rust and the commands to build/test/fmt and how to signoff commits, but i'm definitely looking for something more complete as this guide.

You're missing the part about commits signoff, you can copy it shamelessly from https://github.com/cloudevents/sdk-rust/blob/master/CONTRIBUTING.md#suggesting-a-change

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Signed-off-by: Lance Ball <lball@redhat.com>
@lance
Copy link
Member Author

lance commented May 8, 2020

@slinkydeveloper suggestions applied. Thanks for taking a look.

@grant I removed the branch management stuff - well, pared it way down - and moved it to the "Contributors" section. Also, not sure if you saw my comment earlier:

Do you have some suggested contribution guides that you think we should link to?

Are there specific concerns you have with my proposed workflow suggestions, other than "people don't want to read that"? Is it just the git workflow that you have problems with? Or is it the whole thing? Could you be specific about the sections you object to? I'm happy to accept feedback, but I'm not sure what you mean by simplifying.

@grant
Copy link
Member

grant commented May 8, 2020

@lance Thanks for the improvements. I'm hoping for a simple guide of vital need-to-know information.

Here are some simple guides that we use at Google:

I think a common standard among all SDKs would be ideal as we care about SDK all major languages. A paragraph about a language-specific contribution sounds fine though.

@lance
Copy link
Member Author

lance commented May 8, 2020

@grant OK, I can make those changes - but would like to pull the git instructions out and include a linked step by step guide for those just getting started. I know that when I began contributing to other OSS projects in the past, I personally found these kinds of guides helpful. Sound like a reasonable compromise?

@grant
Copy link
Member

grant commented May 9, 2020

@lance Sounds fine.

  • Things like copying the exact text of developercertificate.org instead of linking it doesn't provide much use.
  • Nifty guides that aren't found elsewhere around git would be helpful.

Copy link
Contributor

@fabiojose fabiojose left a comment

Choose a reason for hiding this comment

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

Good job @lance! It's amazing.

@slinkydeveloper
Copy link
Member

Great job @lance, can you port the same guide in other sdks?

@lance
Copy link
Member Author

lance commented May 11, 2020

@slinkydeveloper sure can - will put it on the todo list.

Signed-off-by: Lance Ball <lball@redhat.com>
@lance
Copy link
Member Author

lance commented May 11, 2020

OK - the PR guidelines were pulled into a separate document, the maintainer tips were pulled out into a second document, and the main contributing doc was greatly simplified. Can I get an approval on this @grant?

@grant
Copy link
Member

grant commented May 11, 2020

OK - the PR guidelines were pulled into a separate document, the maintainer tips were pulled out into a second document, and the main contributing doc was greatly simplified. Can I get an approval on this @grant?

Thanks for the improvements Lance.
Right now I'm looking at all the CE SDKs and can't really approve this PR unless we want the same guidelines in all 7 CE SDKs. In my opinion, this should be dramatically reduced. For example, we shouldn't include common instructions like how to use git.

I'll remove my review request to unblock you though.

@grant grant dismissed their stale review May 11, 2020 17:26

Removing requested changes.

@lance
Copy link
Member Author

lance commented May 11, 2020

@grant does that mean you are not explicitly blocking this from landing, just registering dissent? I'm not clear.

@grant
Copy link
Member

grant commented May 11, 2020

@grant does that mean you are not explicitly blocking this from landing, just registering dissent? I'm not clear.

I just don't approve or request changes. Usually it's not good to merge a PR with requested changes. I just dismissed my review.

As stated I don't really agree with adding any more info about git than is needed. I'd imagine 3-5 bullet points about contributing. I wouldn't imagine a GitHub user to have to read 7 different contributing guides for each SDK.

So, while I can appreciate git guides, style guides, and thank you's in READMEs, it doesn't scale to new developers or hundreds of projects. I'd rather just link a git handbook, use a standard style tool and keep to < 20 lines of code in the README.

@lance
Copy link
Member Author

lance commented May 12, 2020

Now sdk-go has adopted these documents - I'm going to land this.

@lance lance merged commit fd99cb1 into cloudevents:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/docs Module documentation changes type/discussion Issues that need to be decided/debated/discussed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Version Branches Contributor guide is out of date or not enforced
6 participants