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

add PR template to repo #14271

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Conversation

a-mccarthy
Copy link

Signed-off-by: Abigail McCarthy mabigail@vmware.com

Opening this PR as a Draft to start a discussion on adding a PR template to the repo. I'm not sure if this has come up before, but I think these templates can be very helpful to make sure folks fill in some basic info about what the PR is for and remind folks to do the required steps to get the PR merged that can get missed, like the DCO.

I'd also like to make sure we call out updating docs now that the docs are kept in a separate repo.

Is there anything else we should call out here? Like for creating release notes or how to run/pass tests?

Signed-off-by: Abigail McCarthy <mabigail@vmware.com>
@a-mccarthy a-mccarthy marked this pull request as draft February 19, 2021 19:40
@a-mccarthy
Copy link
Author

cc: @xaleeks

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #14271 (93bdf8b) into master (dfe3600) will increase coverage by 2.85%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14271      +/-   ##
==========================================
+ Coverage   62.71%   65.57%   +2.85%     
==========================================
  Files         913      925      +12     
  Lines       61050    69462    +8412     
  Branches     2018     2101      +83     
==========================================
+ Hits        38287    45547    +7260     
- Misses      18828    19964    +1136     
- Partials     3935     3951      +16     
Flag Coverage Δ
unittests 65.57% <ø> (+2.85%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pkg/notification/notification.go 30.76% <0.00%> (-51.84%) ⬇️
src/lib/q/query.go 56.66% <0.00%> (-43.34%) ⬇️
src/controller/quota/options.go 71.42% <0.00%> (-28.58%) ⬇️
...portal/src/app/services/skinable-config.service.ts 54.54% <0.00%> (-25.46%) ⬇️
src/pkg/scan/dao/scanner/registration.go 58.33% <0.00%> (-21.67%) ⬇️
...av/replication/total-replication-page.component.ts 44.82% <0.00%> (-18.81%) ⬇️
src/common/api/base.go 46.42% <0.00%> (-17.86%) ⬇️
src/controller/quota/controller.go 43.53% <0.00%> (-14.29%) ⬇️
src/common/dao/repository.go 62.50% <0.00%> (-13.62%) ⬇️
src/pkg/task/execution.go 62.50% <0.00%> (-8.78%) ⬇️
... and 331 more

@a-mccarthy a-mccarthy marked this pull request as ready for review March 11, 2021 16:27
@a-mccarthy a-mccarthy requested a review from xaleeks March 11, 2021 17:40

### Please indicate you've done the following:

- [ ] [Accepted the DCO](https://github.com/goharbor/harbor/blob/master/CONTRIBUTING.md#commit). Commits without the DCO will delay acceptance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add some other items like testing passed or testing coverage etc. into this checklist?

Or just let the user make sure the CI pipeline is passed?

### Please indicate you've done the following:

- [ ] [Accepted the DCO](https://github.com/goharbor/harbor/blob/master/CONTRIBUTING.md#commit). Commits without the DCO will delay acceptance.
- [ ] Updated the corresponding documentation in this repo or https://github.com/goharbor/website.
Copy link

Choose a reason for hiding this comment

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

Thanks Abbie for creating the template. Assuming this one is for those PRs need document change. If the PR doesn't need document change, how submitter make the choice. And it would be better to change the document after this PR is merged.
One suggestion is to make it as a note only.

Copy link
Author

Choose a reason for hiding this comment

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

Thats a good point. My first goal is to remind folks about updating the docs. I think my concern with making it a note, instead of a check mark is that there is no sign-off from a contributor.

What if we made it more open ended? something like,

- [ ] Considered the docs impact and opened a new docs issue or PR with docs changes if needed

My concern is that if we merge a PR without an option like this as a part of the checklist, then there will be no way to remember to open and docs issue or PR after the pr is merged. I'm interested to hear thoughts and feedback on this idea

Copy link

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Abigail McCarthy <mabigail@vmware.com>
@reasonerjt reasonerjt merged commit a8622a3 into goharbor:master Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants