[Pulp-dev] Github Required Checks

[Daniel Alley]

I don't think we should make it a hard *physical* block on PR merging.
Setting aside the occasional infrastructure issues, we also have some unit
tests (in pulp core, at least) that rely on e.g. non-expired certificates,
and fixing those once they break would require circumventing the process or
disabling the checks.  Maybe that scenario is infrequent enough that we
don't care, though - I can see that being the case.

I _do_ think we need to formalize a set of rules about merging, though, and
decide how strict we want to be about it.  There are a few possibilities:


Option 1:  Nothing merges without passing PR runner tests, ever, even if
the issue is rooted in the configuration or infrastructure of the test
runners or an expired certificate etc.  This would light a fire to get
these issues resolved ASAP because nothing can happen without them.

Option 2:  Every PR must pass all the unit tests, have a clean docs build
and no PEP8 errors, but if the automated runners are not working correctly
it is fine to just run those tests offline and merge if they pass.

Option 3:  Every PR must pass all the unit tests, but if something is wrong
with the automated docs or PEP8 runners, we disregard them until they are
functional again and fix anything that got through in the interim.

(etc.)

I think using a PEP for this would be reasonable.


As an aside, you mentioned that we already have the required checks enabled
on the 3.0-dev branch, but I'm pretty sure we don't.  See this [0] PR where
the automated docs builder failed to run properly (not a docs issue) but
was merged.

[0] https://github.com/pulp/pulp/pull/3280

On Fri, Feb 2, 2018 at 12:34 PM, Brian Bouterse <bbouters at redhat.com> wrote:

> A week or two ago on irc, @misa identified that unit tests which run on
> travis with crane prs had been failing for a long time. These unit tests
> are run on each PR that is submitted, but broken and ignored. We were
> interested in finding a better way to ensure that our required checks all
> must pass before a PR can be merged.
>
> I want to get feedback on an idea to resolve this. Github has settings
> which restricts merging until the required checks are met. It allows you to:
>
> - required review before merging
> - reviewed status checks passing before merging. For us this would include
> unit test runs and docs builder runs
>
> What if we enable ^ on the 'master' branch of all the dev maintained repos
> including:
>
> devel, pulp-ci, pulp, pulp_file, pulp_puppet, pulp_rpm, pulp_docker,
> crane, ansible-pulp3, pulp_template, pulp_example, pulpproject.org,
> pulp_python, pulp_deb, ansible-pulp3_systemd, pulp_ostree, ansible-pulp3_db
>
> We have already enabled this on the 3.0-dev branch in the pulp repo maybe
> 6 months ago, so this would be more of the same.
>
> What do you all think about this idea? What questions or alternatives are
> there? Should this be done via a pup?
>
> Any feedback or ideas are welcome.
>
> -Brian
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20180202/f4f2f190/attachment.htm>