[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Pulp-dev] Github Required Checks



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 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 redhat com
https://www.redhat.com/mailman/listinfo/pulp-dev



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]