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

Adds Contrib Doc #27

Merged
merged 4 commits into from
May 3, 2022
Merged

Adds Contrib Doc #27

merged 4 commits into from
May 3, 2022

Conversation

danehans
Copy link
Contributor

Adds a contrib doc based on Envoy and go-control-plane.

Fixes: #8

Signed-off-by: danehans daneyonhansen@gmail.com

CONTRIBUTING.md Outdated
Comment on lines 78 to 79
* It is generally expected that a maintainer representing a different organization from the PR owner
is required to review the PR.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LukeShu @youngnick note that this topic was discussed during a community meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would use even stronger language and make it a requirement instead of a "general expectation". Perhaps we should also use the term "affiliation" rather than "organization" to distinguish it from GitHub orgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my response in #27 (comment)

youngnick
youngnick previously approved these changes Apr 29, 2022
@danehans
Copy link
Contributor Author

@LukeShu I would like all 3 organizations to review PRs during the early stages of the project, so PTAL and merge if all looks good to you.

@danehans danehans added the documentation Improvements or additions to documentation label Apr 29, 2022
@danehans danehans added this to the Public Announcement milestone Apr 29, 2022
@danehans danehans mentioned this pull request May 2, 2022
7 tasks
arkodg
arkodg previously approved these changes May 2, 2022

## Communication

* Before starting work on a major feature, please contact us via GitHub or Slack. We will ensure no
Copy link
Contributor

Choose a reason for hiding this comment

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

We should provide links to GitHub and Slack as soon as the project and channels are made public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they will be added to the readme, xref.

build. If your PR cannot have 100% coverage for some reason please clearly explain why when you
open it.
* Any PR that changes user-facing behavior **must** have associated documentation in [docs](docs) as
well as the [changelog](CHANGELOG.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

CHANGELOG.md doesn't exist yet.
xref: #54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for creating the issue!

CONTRIBUTING.md Outdated
* When all tests are passing and all other conditions described herein are satisfied, a maintainer
will be assigned to review and merge the PR.
* Once you submit a PR, *please do not rebase it*. It's much easier to review if subsequent commits
are new commits and/or merges. We squash rebase the final merged commit so the number of commits
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change the repository's configuration to make "Squash and merge" the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

CONTRIBUTING.md Outdated
Comment on lines 78 to 79
* It is generally expected that a maintainer representing a different organization from the PR owner
is required to review the PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use even stronger language and make it a requirement instead of a "general expectation". Perhaps we should also use the term "affiliation" rather than "organization" to distinguish it from GitHub orgs.

@danehans
Copy link
Contributor Author

danehans commented May 3, 2022

I would use even stronger language and make it a requirement instead of a "general expectation". Perhaps we should also use the term "affiliation" rather than "organization" to distinguish it from GitHub orgs.

I will update to use "affiliation" instead of "organization". I use "general expectation" due to the following caveat:

  • The above rules may be waived for PRs which only update docs or comments, or trivial changes to
    tests and tools (where trivial is decided by the maintainer in question).

I have updated this bullet to clearly state this is required, PTAL.

cc: @youngnick @skriss @arkodg @LukeShu

@danehans danehans dismissed stale reviews from arkodg and youngnick via 40777da May 3, 2022 15:54
@danehans danehans force-pushed the issue_8 branch 2 times, most recently from 40777da to 1163c07 Compare May 3, 2022 15:57
alexgervais
alexgervais previously approved these changes May 3, 2022
skriss
skriss previously approved these changes May 3, 2022
Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Contributor Author

danehans commented May 3, 2022

Had to rebase. PTAL and /approve if all looks good.

cc: @skriss @youngnick @alexgervais @alexgervais

Copy link
Contributor

@LukeShu LukeShu left a comment

Choose a reason for hiding this comment

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

I have no problems with this.

@danehans danehans merged commit 52504ff into main May 3, 2022
@danehans danehans deleted the issue_8 branch May 3, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Contrib Guide
6 participants