-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add basic pull request template and update documentation regarding ad… #69
Conversation
PULL_REQUEST_TEMPLATE.md
Outdated
|
||
# Checklist: | ||
|
||
Please select all options that was done as part of this change. |
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.
Should this be Please complete all steps before opening a pull request.
and then a numbered list?
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.
If you don't want to go the numbered list route, this still needs rewording. If the steps are mandatory, they should not be referred to as options, and it should read "were done" instead of "was done". I'd suggest:
Confirm you have completed the following steps:
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.
updated
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.
however the only time not all steps are mandatory is during document update but I guess thats ok
PULL_REQUEST_TEMPLATE.md
Outdated
|
||
Please select all options that was done as part of this change. | ||
|
||
- [ ] My code follows the style guidelines of this project |
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.
remove guidelines
, and just state My code follows the style of this project
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.
updated
README.md
Outdated
@@ -139,6 +139,8 @@ The following [dispatchers](./telemetry/producer.go#L10-L19) are supported | |||
* Override stream names with env variables: KINESIS_STREAM_\*uppercase topic\* ex.: `KINESIS_STREAM_V` | |||
* Google pubsub: Along with the required pubsub config (See ./test/integration/config.json for example), be sure to set the environment variable `GOOGLE_APPLICATION_CREDENTIALS` | |||
* Logger: This is a simple STDOUT logger that serializes the protos to json. | |||
|
|||
>NOTE: If you want to add a new dispatcher, it must have an integration test and updated documentation associated with it. |
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.
NOTE: To add a new dispatcher, please provide integration tests and updated documentation.
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.
updated
PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] My code follows the style guidelines of this project | ||
- [ ] I have performed a self-review of my code | ||
- [ ] I have made corresponding updates to the documentation | ||
- [ ] Unit tests are added/updated to cover my changes |
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.
This mixes passive and active voice. Consider beginning all sentences with "I have..." for consistency. E.g,. "I have added/updated unit tests to cover my changes."
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.
make sense updated
8e35814
to
2d768b1
Compare
PULL_REQUEST_TEMPLATE.md
Outdated
|
||
- [ ] My code follows the style of this project | ||
- [ ] I have performed a self-review of my code | ||
- [ ] I have made corresponding updates to the documentation |
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.
nit: 1-3 don't end with a period, 4-5 do end with a period
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.
addressed
# Checklist: | ||
|
||
Confirm you have completed the following steps: | ||
|
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.
Nit: Half of these have periods and half don't.
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.
addressed
…ding a new producer
2d768b1
to
604e6b3
Compare
…ding a new producer