Today in the foreman retrospective some discussions came up about how to
reduce the backlog of prs. Below is some feedback I have gotten about
contributing to Foreman and Katello. I dont bring this up to blame or
gripe at folks, but to kickoff to the discussion about how to make the
PR process quicker and the abandon rate lower.
The katello contribution notes are related to the katello-installer
which I realize is a PiTA.
The longer a PR stays open, the more folks chime in which leads to
more style comments ".select() is better than .each().". Either having a
style guide, or being able to only listen to one reviewer would be nice.
Leads to alot of churn.
One PR fixed a particular itch, but it was rejected since it did not
fix the same problem in all examples. The submitter abandoned it. He
would have preferred to commit his fix and then open an issue for the rest.
This comment goes to support Ohad's comment about making changes: "I
submitted the PR and then kept getting nickled and dimed over the commit
message, the Redmine issue, which repo to add to, etc. If there had
been one document that had listed all the steps, I could have done them
all at once and I would have been a lot less annoyed. Or the team could
had taken my PR and then moved it through the process and taught me
about it as they did it. Overall, it just gave me the impression that my
contribution wasn't really wanted and they were doing me a favor by even
considering it."
Do folks have similar issues? Are there docs which need to be improved
or more easily located? Are there other issues which cause devs to
abandon prs? Thanks!
> Today in the foreman retrospective some discussions came up about how to
> reduce the backlog of prs. Below is some feedback I have gotten about
> contributing to Foreman and Katello. I dont bring this up to blame or
> gripe at folks, but to kickoff to the discussion about how to make the
> PR process quicker and the abandon rate lower.
>
> The katello contribution notes are related to the katello-installer
> which I realize is a PiTA.
>
> * The longer a PR stays open, the more folks chime in which leads to
> more style comments ".select() is better than .each().". Either having a
> style guide, or being able to only listen to one reviewer would be nice.
> Leads to alot of churn.
The code base has more different code styles than it has source files
right now, but I'd definitely be in favour of fixing this. I'd like to
see Rubocop applied, but it's going to require a significant and
concerted effort. Feature #3809: Add rubocop to foreman - Foreman remains
untouched.
I'd rather not suggest we only have one reviewer, but I appreciate the
problem exists.
> * One PR fixed a particular itch, but it was rejected since it did not
> fix the same problem in all examples. The submitter abandoned it. He
> would have preferred to commit his fix and then open an issue for the rest.
I tend to consider who's making the PR as to whether to enforce this,
but in most cases I will continue to expect developers to solve the
problem completely. First time contributors, or people who I realise
are at their limit get leeway and we'll file an issue for the completion
of the task.
Generally I think we've found committing an incomplete solution leads to
more work (including admin, e.g. triaging tickets for similar issues,
planning the additional ticket) and makes it harder to release - as we'd
generally want the complete fix so the user experience is consistent.
Certainly just in the last two weeks I've triaged a lot of near
identical tickets (in Bugzilla and Redmine).
> * This comment goes to support Ohad's comment about making changes: "I
> submitted the PR and then kept getting nickled and dimed over the commit
> message, the Redmine issue, which repo to add to, etc. If there had
> been one document that had listed all the steps, I could have done them
> all at once and I would have been a lot less annoyed. Or the team could
> had taken my PR and then moved it through the process and taught me
> about it as they did it. Overall, it just gave me the impression that my
> contribution wasn't really wanted and they were doing me a favor by even
> considering it."
Again for new contributors we frequently handle the process on their
behalf, but I'll admit that I expect other folks (by which I mean Red
Hat employees, as I guess you do) to be more proactive.
> Do folks have similar issues? Are there docs which need to be improved
> or more easily located? Are there other issues which cause devs to
> abandon prs? Thanks!
I'd appreciate improvements to the contribution document on the website,
including linking to the above two docs: http://theforeman.org/contribute.html
Updating this with the required steps would help a lot.
+1 to rubocop. We've been very happy with it in Katello and I feel like as a reviewer and a code committer, it has made my job a lot easier since PR comments are almost style-free. There are still some places that aren't checked by rubocop in our code though. It would be nice to get those places checked.
Also, if a PR has a "Needs Re-Review", I personally try to avoid bringing up small style issues. I think the label is going to help to identify these cases and I think it should be a good rule of thumb.
Lastly, Katello has started documenting these things. For example, here is our documentation on how to write a good commit message:
···
----- Original Message -----
> From: "Dominic Cleal"
> To: foreman-dev@googlegroups.com
> Sent: Friday, August 1, 2014 12:17:33 PM
> Subject: Re: [foreman-dev] Feedback on getting contributions into the code
>
> On 01/08/14 16:56, Bryan Kearney wrote:
> > Today in the foreman retrospective some discussions came up about how to
> > reduce the backlog of prs. Below is some feedback I have gotten about
> > contributing to Foreman and Katello. I dont bring this up to blame or
> > gripe at folks, but to kickoff to the discussion about how to make the
> > PR process quicker and the abandon rate lower.
> >
> > The katello contribution notes are related to the katello-installer
> > which I realize is a PiTA.
> >
> > * The longer a PR stays open, the more folks chime in which leads to
> > more style comments ".select() is better than .each().". Either having a
> > style guide, or being able to only listen to one reviewer would be nice.
> > Leads to alot of churn.
>
> The code base has more different code styles than it has source files
> right now, but I'd definitely be in favour of fixing this. I'd like to
> see Rubocop applied, but it's going to require a significant and
> concerted effort. http://projects.theforeman.org/issues/3809 remains
> untouched.
>
> I'd rather not suggest we only have one reviewer, but I appreciate the
> problem exists.
>
> http://projects.theforeman.org/projects/foreman/wiki/Reviewing_patches
> covers some style issues which will be checked during reviews, but it's
> great if contributors can be aware of them first.
>
> > * One PR fixed a particular itch, but it was rejected since it did not
> > fix the same problem in all examples. The submitter abandoned it. He
> > would have preferred to commit his fix and then open an issue for the rest.
>
> I tend to consider who's making the PR as to whether to enforce this,
> but in most cases I will continue to expect developers to solve the
> problem completely. First time contributors, or people who I realise
> are at their limit get leeway and we'll file an issue for the completion
> of the task.
>
> Generally I think we've found committing an incomplete solution leads to
> more work (including admin, e.g. triaging tickets for similar issues,
> planning the additional ticket) and makes it harder to release - as we'd
> generally want the complete fix so the user experience is consistent.
> Certainly just in the last two weeks I've triaged a *lot* of near
> identical tickets (in Bugzilla and Redmine).
>
> > * This comment goes to support Ohad's comment about making changes: "I
> > submitted the PR and then kept getting nickled and dimed over the commit
> > message, the Redmine issue, which repo to add to, etc. If there had
> > been one document that had listed all the steps, I could have done them
> > all at once and I would have been a lot less annoyed. Or the team could
> > had taken my PR and then moved it through the process and taught me
> > about it as they did it. Overall, it just gave me the impression that my
> > contribution wasn't really wanted and they were doing me a favor by even
> > considering it."
>
> Ohad wrote up this wiki page recently about commit messages, which I
> believe is accurate and can be used as a reference:
> http://projects.theforeman.org/projects/foreman/wiki/Reviewing_patches-commit_message_format
>
> Again for new contributors we frequently handle the process on their
> behalf, but I'll admit that I expect other folks (by which I mean Red
> Hat employees, as I guess you do) to be more proactive.
>
> > Do folks have similar issues? Are there docs which need to be improved
> > or more easily located? Are there other issues which cause devs to
> > abandon prs? Thanks!
>
> I'd appreciate improvements to the contribution document on the website,
> including linking to the above two docs:
> http://theforeman.org/contribute.html
> https://github.com/theforeman/theforeman.org/blob/gh-pages/contribute.md
>
> Updating this with the required steps would help a lot.
>
> Thanks,
>
> --
> Dominic Cleal
> Red Hat Engineering
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
···
----- Original Message -----
> From: "David Davis"
> To: foreman-dev@googlegroups.com
> Sent: Friday, August 1, 2014 2:15:08 PM
> Subject: Re: [foreman-dev] Feedback on getting contributions into the code
>
> +1 to rubocop. We've been very happy with it in Katello and I feel like as a
> reviewer and a code committer, it has made my job a lot easier since PR
> comments are almost style-free. There are still some places that aren't
> checked by rubocop in our code though. It would be nice to get those places
> checked.
>
> Also, if a PR has a "Needs Re-Review", I personally try to avoid bringing up
> small style issues. I think the label is going to help to identify these
> cases and I think it should be a good rule of thumb.
>
> Lastly, Katello has started documenting these things. For example, here is
> our documentation on how to write a good commit message:
>
> https://github.com/Katello/katello.org/blob/master/docs/developer_guide/style/git_commit_messages.mkd
>
> Also, we even have a PR checklist:
>
> https://github.com/Katello/katello.org/blob/master/docs/developer_guide/pull_request_checklist.mkd
>
> Does Foreman not have something similar?
>
> David
>
> ----- Original Message -----
> > From: "Dominic Cleal"
> > To: foreman-dev@googlegroups.com
> > Sent: Friday, August 1, 2014 12:17:33 PM
> > Subject: Re: [foreman-dev] Feedback on getting contributions into the code
> >
> > On 01/08/14 16:56, Bryan Kearney wrote:
> > > Today in the foreman retrospective some discussions came up about how to
> > > reduce the backlog of prs. Below is some feedback I have gotten about
> > > contributing to Foreman and Katello. I dont bring this up to blame or
> > > gripe at folks, but to kickoff to the discussion about how to make the
> > > PR process quicker and the abandon rate lower.
> > >
> > > The katello contribution notes are related to the katello-installer
> > > which I realize is a PiTA.
> > >
> > > * The longer a PR stays open, the more folks chime in which leads to
> > > more style comments ".select() is better than .each().". Either having a
> > > style guide, or being able to only listen to one reviewer would be nice.
> > > Leads to alot of churn.
> >
> > The code base has more different code styles than it has source files
> > right now, but I'd definitely be in favour of fixing this. I'd like to
> > see Rubocop applied, but it's going to require a significant and
> > concerted effort. http://projects.theforeman.org/issues/3809 remains
> > untouched.
> >
> > I'd rather not suggest we only have one reviewer, but I appreciate the
> > problem exists.
> >
> > http://projects.theforeman.org/projects/foreman/wiki/Reviewing_patches
> > covers some style issues which will be checked during reviews, but it's
> > great if contributors can be aware of them first.
> >
> > > * One PR fixed a particular itch, but it was rejected since it did not
> > > fix the same problem in all examples. The submitter abandoned it. He
> > > would have preferred to commit his fix and then open an issue for the
> > > rest.
> >
> > I tend to consider who's making the PR as to whether to enforce this,
> > but in most cases I will continue to expect developers to solve the
> > problem completely. First time contributors, or people who I realise
> > are at their limit get leeway and we'll file an issue for the completion
> > of the task.
> >
> > Generally I think we've found committing an incomplete solution leads to
> > more work (including admin, e.g. triaging tickets for similar issues,
> > planning the additional ticket) and makes it harder to release - as we'd
> > generally want the complete fix so the user experience is consistent.
> > Certainly just in the last two weeks I've triaged a *lot* of near
> > identical tickets (in Bugzilla and Redmine).
> >
> > > * This comment goes to support Ohad's comment about making changes: "I
> > > submitted the PR and then kept getting nickled and dimed over the commit
> > > message, the Redmine issue, which repo to add to, etc. If there had
> > > been one document that had listed all the steps, I could have done them
> > > all at once and I would have been a lot less annoyed. Or the team could
> > > had taken my PR and then moved it through the process and taught me
> > > about it as they did it. Overall, it just gave me the impression that my
> > > contribution wasn't really wanted and they were doing me a favor by even
> > > considering it."
> >
> > Ohad wrote up this wiki page recently about commit messages, which I
> > believe is accurate and can be used as a reference:
> > http://projects.theforeman.org/projects/foreman/wiki/Reviewing_patches-commit_message_format
> >
> > Again for new contributors we frequently handle the process on their
> > behalf, but I'll admit that I expect other folks (by which I mean Red
> > Hat employees, as I guess you do) to be more proactive.
> >
> > > Do folks have similar issues? Are there docs which need to be improved
> > > or more easily located? Are there other issues which cause devs to
> > > abandon prs? Thanks!
> >
> > I'd appreciate improvements to the contribution document on the website,
> > including linking to the above two docs:
> > http://theforeman.org/contribute.html
> > https://github.com/theforeman/theforeman.org/blob/gh-pages/contribute.md
> >
> > Updating this with the required steps would help a lot.
> >
> > Thanks,
> >
> > --
> > Dominic Cleal
> > Red Hat Engineering
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "foreman-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to foreman-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
> I'd rather not suggest we only have one reviewer, but I appreciate the
> problem exists.
>
> Reviewing patches - Foreman
> covers some style issues which will be checked during reviews, but it's
> great if contributors can be aware of them first.
I think I tried several times to include these tickboxes with the
review. Do you think it would make sense that the bot would
automatically add this to all of the incoming PRs?
If not, I will hack something similar to fedora review, because I always
forget things when doing reviews.
