Repository navigation
Feature request: Ability to turn off all CI for draft PR #10729
Description
Extracting from #10706 - it might be useful to have an easy way to turn off CI - instead of or in addition to the [CI skip] we can use now. In particular, might it be possible to link it to a draft PR? Often those are used to give people a sense of how things would look rather than do complete tests (and if it is linked, a quick change to regular PR would run CI).
Not sure if this is possible and not high priority, but perhaps someone just knows how to do it...
Activity
pllim commentedon Sep 14, 2020
One way this can be implemented is to have the CI recognize some special label that is applied to the PR. If (or unless, depending on how you want to logic to work) the label is found, CI will do a no-op.
In fact, there is a feature request for this on Travis CI but doesn't seem resolved: travis-ci/travis-ci#5374
pllim commentedon Sep 14, 2020
p.s. Travis CI used to not run for Draft PR but they fixed it.
Personally, I want CI to run on my draft/experimental PR. I want to know if they work even if I don't want them merged, to check for one thing or another.
bsipocz commentedon Sep 14, 2020
@mhvk - what is wrong with
[skip ci]? Sometimes it makes sense to have the CI run on a draft, sometimes it doesn't. I really trust the contributors are reading the guides and use the current skip mechanism. And if they push too many commits triggering CI, then they will react to a polite ask to change their practices to either push less frequently, or use the skips.Also, how would you retrigger a CI build based on changing a PR from 'draft' to 'normal'?
mhvk commentedon Sep 14, 2020
@pllim - I think generally for a draft PR it is good enough to know that things passed locally, but I'll admit that I've found problems by CI being run. I'm imagining a situation where you can trigger CI at will with a quick Draft PR -> PR -> Draft PR change...
@bsipocz - nothing really wrong with
[skip ci], just that I know for myself that I regularly forget it, and then have to get into travis/circle-ci to explicitly cancel jobs. I also fear that from reviewing PRs I cannot share your trust that contributors can be trusted to read the manual on this... (or on not to blacken, etc., etc.).Anyway, I think it would be useful to have a mechanism to say, hey, I've turned of CI for now, but you can turn it back on yourself the moment you feel ready. And I think PR <-> draft PR can be done by the submitter, unlike assigning labels, so that may be an advantage.
As for how to trigger in detail, not sure, but certainly adding a milestone to a PR triggers astrobot to redo its changelog check, so it would seem to be possible...
bsipocz commentedon Sep 14, 2020
yes, I certainly share this experience and feeling, but would very much like to get to a place where it's acceptable for the maintainers to point these shortcomings out rather than go into an argument about those and let the contributor do whatever they see fit [*]. Also, I would very much like a world where people do read the manual first rather than expect hand holding without putting in any efforts, but apparently I'm in the minority with that expectation. (And to be clear, I think hand holding should be offered, but only after the homework is done and ready. I yet to come across someone who became a long term contributor for any project who didn't bother to look into the documents but expected to be spoon fed and most of all we need the new generation of long term contributors rather than occasional fly-bys).
[*] E.g. if we say no black then no black please, it has been discussed and there most probably there are reasons for that decisions. It's rarely that none of the maintainers have come across of a hyped tool a first timer tries to push on the project because they find it convenient. "Oh, but I don't know how to revert my blacked changes" is not an excuse we should ever except.
mhvk commentedon Sep 14, 2020
@bsipocz - just to be clear, I couldn't agree more that people should read the manual! But it never hurts to make things easy and automatic - as a reviewer, it makes it easier to be welcoming (e.g., this is why the PEP8 check is so good to have - remember how annoying it was to have to educate contributors about that?).
bsipocz commentedon Sep 14, 2020
Having that PEP8 bot took us 2 years to achieve, I remember I first suggested to use it in the NYC coordination meeting, then we had to run it with astroquery for testing and to make the case for those opposing, and finally it got opted in for core, very reluctantly, last year in Socorro. It would be nice to quicken adaptation of things in the future.
I'm on team "more check and CI" in general, and if it was me, there would be more strict PEP8 (it could also be done with a pre-commit hook, or just a very strict settings list, no preference there), but it's not the same as random more things that individuals prefer even when maintainers say it's less legible for them for long term maintenance. Then it shouldn't be ever "but you get used to it in a few weeks time". I would really love to see that sustainability, maintainability, and maintainers start to matter.
mhvk commentedon Sep 14, 2020
Indeed, the PEP8 change took long, partially for fear about being unwelcoming. I think this has proven to be false, thankfully, and indeed now I would see it as a prime example of the benefit of something automatic in reducing friction.
But I'm less sure how we can make this happen automatically for the code style otherwise...
pllim commentedon Sep 14, 2020
Unless the CI service natively supports this feature or if it can be customized in less than 10 lines of code, I don't see the point in us trying to implement it from scratch.
My 👎 here is based on what I think is a disagreement at a more fundamental level of what we think a Draft PR is for. I disagree that CI should not run for Draft PR. I take advantage of the CI on my draft PRs all the time. I don't want people to merge it too soon, but I am working on legit code that should not break the tests. Like Brigitta said, contributors already have options to turn CI off and they need to do the due diligence to learn how to do what is possible already.
bsipocz commentedon Sep 14, 2020
And this is where the project could have listened to the data that we had at hand from other projects, as well as from those who've been on the ground helping the newcomers (thus have the friction from both the new contributors and people on the project talking about new contributors all the time yet never spend any time in mentoring them).
Both data and people on the ground of course are ignored in favour of theories of what is unwelcoming.
mhvk commentedon Sep 15, 2020
We might indeed benefit from some mentoring of the reviewers, but that is getting too off-topic here. Hopefully, the funding and associated paid work will alleviate some of the problems - I'd certainly quite happily hand over some responsibilities myself.
On topic: seeing reasonable objections and not much in support, I think the status quo wins! So, closing...
pllim commentedon Sep 15, 2020
Sorry, @mhvk ! If the situation changes, we can always revisit or brainstorm some alternatives, especially if Travis CI provides native support for this feature. I mean, I would be happy to disable CI for certain PRs but not all Draft PRs should be automatically penalized because of the few. Also, when that feature is introduced, we also need a written policy on when it should be deemed necessary (we also need written policy of some of the stuff we already do, like labeling etc, but that is another story).
astrofrog commentedon Sep 15, 2020
There is one thing which we could do which is that if a draft PR is opened, astropy-bot could leave a comment to tell people to remember to add [ci skip] if they don’t want CI to run? (And tha it will run otherwise)
pllim commentedon Sep 15, 2020
Ohh... @astrofrog , that is a good idea!
pllim commentedon Sep 15, 2020
@astrofrog , seems generic enough of a functionality that it should be in
baldrick?pllim commentedon Sep 15, 2020
Or maybe even a GitHub Action! Choices, choices.
astrofrog commentedon Sep 15, 2020
Yeah it could be a plug-in that lives in baldrick and gets activated in astropy bot. Do you agree @Cadair?
astrofrog commentedon Sep 15, 2020
Well we have astropy bot already set up so not sure I want to add a parallel set of bot things on actions...
pllim commentedon Sep 15, 2020
Well... I am leaning against a POC using Actions. 😬
pllim commentedon Sep 15, 2020
... because it is fun. See #10733
astrofrog commentedon Sep 15, 2020
But then it has to be set up for each repo... are you sure you want to add a new box to your infrastructure diagram?...
pllim commentedon Sep 15, 2020
We are not even sure if we want this for all the repos yet. Let's take this to Slack. I think we're getting way off topic here. 😅
mhvk commentedon Sep 15, 2020
Just a 👍 to the comment-by-bot -- if it is easy!