search

vivo-community / vivo-graphql

graphql endpoint for the vivo scholar project
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

Review development process #4

Closed outten45 closed 5 years ago

outten45 commented 5 years ago

Use this issue to capture feedback on the initial start to the development process. Please leave a comment below if you have feedback.

https://github.com/vivo-community/vivo-graphql/wiki/Development-Process

nymbyl commented 5 years ago

I think a naming convention is good - the "s[sprint#]i[issue#]_" is maybe a little cryptic, but should work.

Having the author merge their own requests seems okay to me for a limited period of time e.g. in the "development" phase of a project. Since nothing is shipping at that point, I feel like the risks are lower.

I wondering how the things will go as far as announcing a pull request on slack and waiting for a +1 to happen. I guess it depends on how often that happens and how bunched together they get etc... probably fine.

outten45 commented 5 years ago

hi @nymbyl maybe we should simplify the naming of the branch. what about?

{issue number}_descriptive_name

I think I was talking with @jimwood and came up with that more cryptic naming convention (my bad :slightly_frowning_face:).

as for the +1 on slack, we can make adjustments as we go.

nymbyl commented 5 years ago

@outten45 - a simpler name might work - maybe the sprint number is not that useful for sorting or searching. But I am all for adding the "[issue#]" in the commit message

awoods commented 5 years ago

I generally get nervous in the face of any GitHub process that involves an author merging their own pull-request. Would a middle-ground be where the reviewer does the merge?

It may also be clearer if instead of a :+1: in the comments, using the GitHub "approval process".

outten45 commented 5 years ago

hi @awoods I like the "requiring approval" approach (our gitlab instance has that). thanks for pointing it out - I didn't realize github had it. I added a branch rule to require 1 approver to see how it works (we can always remove/tweak).

leaving this issue open for others to comment.

outten45 commented 5 years ago

based on the feedback already, I've updated the documentation. I'll leave this issue open until Friday in case others have comments. The 3 changes I made:

1) branch naming 2) using the github approval process 3) the reviewer merges the branch

https://github.com/vivo-community/vivo-graphql/wiki/Development-Process