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

chore: add issue and pull request templates #515

Merged
merged 4 commits into from
Dec 4, 2020

Conversation

kushthedude
Copy link
Contributor

Signed-off-by: Kush Trivedi kushthedude@gmail.com

/cc @oschaaf

Signed-off-by: Kush Trivedi <kushthedude@gmail.com>
Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!
I'm curious, is there a way to preview the effects of this before merging?
I have a suspicion I know the answer to that (no), but I thought lets ask anyway.

As for the substance of this PR, leaving one ask, other than that this LGTM.

I'll defer to @dubious90 for final review.


Contributing Conventions:

1. Build and test your changes before submitting a PR.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you are right, also we can mention the DCO requirement I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, yes. There's also pre-push/pre-commit hooks in https://github.com/envoyproxy/nighthawk/tree/master/support that may help with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd actually argue further to Otto's point that I'd prefer we not duplicate where our contributing conventions are. I'd prefer to only reference the other markdown file here rather than specify some conventions and reference it here.

I could probably be convinced in a different direction if you feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we can reference the markdown file here than the other way around.

@oschaaf
Copy link
Member

oschaaf commented Sep 8, 2020

/assign dubious90

@oschaaf oschaaf added the waiting-for-review A PR waiting for a review. label Sep 8, 2020
Copy link
Contributor

@dubious90 dubious90 left a comment

Choose a reason for hiding this comment

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

This looks awesome. Thanks! Just a couple suggestions

@@ -0,0 +1,16 @@
**Description**

This PR fixes #
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we edit this language a little? I appreciate encouraging people to link to issues, but I don't think we want a 1:1 of issues to PRs, because it encourages longer PRs that are harder to review.

Maybe This PR is related to.

I'm also curious whether we should include anything about this template being a guideline, rather than set in stone. For instance, this PR is not related to an ongoing issue, and I'm not sure we would've gotten any benefit by suggesting that you should have created one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add a comment saying if the PR is related to an issue then mention it below


Contributing Conventions:

1. Build and test your changes before submitting a 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'd actually argue further to Otto's point that I'd prefer we not duplicate where our contributing conventions are. I'd prefer to only reference the other markdown file here rather than specify some conventions and reference it here.

I could probably be convinced in a different direction if you feel strongly.

@dubious90 dubious90 added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 8, 2020
@oschaaf
Copy link
Member

oschaaf commented Sep 24, 2020

@kushthedude kindly pinging this PR, there is an open question in the review comments

Signed-off-by: Kush Trivedi <kushthedude@gmail.com>
@kushthedude
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #515 (comment) was created by @kushthedude.

see: more, trace.

Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

LGTM

@dubious90 dubious90 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Nov 9, 2020
@oschaaf
Copy link
Member

oschaaf commented Nov 9, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #515 (comment) was created by @oschaaf.

see: more, trace.

@mum4k
Copy link
Collaborator

mum4k commented Nov 11, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: test (failed build)

🐱

Caused by: a #515 (comment) was created by @mum4k.

see: more, trace.

@mum4k
Copy link
Collaborator

mum4k commented Nov 11, 2020

Ready for merging, blocked on the clang_tidy flake which we are planning to remove temporarily in the near future (#570).

@mum4k
Copy link
Collaborator

mum4k commented Nov 11, 2020

Please merge from master to remove the hard block on clang_tidy.

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Nov 11, 2020
@kushthedude kushthedude requested a review from dubious90 December 4, 2020 15:52
@kushthedude
Copy link
Contributor Author

Please merge from master to remove the hard block on clang_tidy.

Done @mum4k

@dubious90 dubious90 merged commit 2afa57b into envoyproxy:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes A PR waiting for comments to be resolved and changes to be applied.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants