Feedback on the new CPython workflow
Hi,
I wanted to wait a little bit before giving back my feedback on the new workflow. I just attend Brett Canon's talk at the Language Summit. So here are my misc notes on the new workflow.
Is there anyone already working on the workflow who would like to get a grant (money!) from the PSF?
If I want to share a change to review but it must not be merged, would it be possible to prevent merge by mistake? Maybe add [WIP] to the title and modify our bot to mark the "build" as failed? For example, I wrote a patch which depends on a different patch but I didn't want to create a patch serie since it didn't make sense.
AppVeyor is now quite stable. I fixed a few unstable tests. I suggest to mark this CI as mandatory. The CI is as fast or even faster than Travis CI! But it seems like it accepts less jobs in parallel :-/ It might be an issue if we get a huge number of PR in a short period (ex: during an event like Pycon US).
Currently, cherry-picker works a single step. It would be nice to have at least 2 steps: first cherry-pick locally, then allow to review the patch locally and run some specific tests, and then send the PR. By the way, instead of only using the sha1 to build the branch name, it would be nice to add also the bpo number.
I suggest to track backports at bpo since an issue tracks all PR and it seems like most core developers want to put most informations in the issue rather than GitHub.
Find a way to keep the Code coverage job, but don't mark the PR as failed if the coverage is considered as "failed"? It's strange to see a long list of PR with the red sign (failed) whereas Travis and AppVeyor passed.
I have a lot of other minor remarks, but I prefer to stop here ;-)
Victor
- Currently, cherry-picker works a single step. It would be nice to have at least 2 steps: first cherry-pick locally, then allow to review the patch locally and run some specific tests, and then send the PR.
The --no-push parameter allows you to test changes locally first. Then you can use --continue to finish the backport and open the PR:
Discussion: https://github.com/python/core-workflow/issues/78 Implementation: https://github.com/python/core-workflow/pull/84
By the way, instead of only using the sha1 to build the branch name,
it would be nice to add also the bpo number.
It's possible, but remember not all PRs have bpo-issue, eg those with
trivial label.
In that case, what should the backport branch be?
So we might end up with two backport branch name patterns, eg
backport-bpo-NNNN- and backport-sha1 for the trivial PRs ?
Thanks :)
Mariatta Wijaya
On Wed, May 17, 2017 at 11:26 AM, Victor Stinner <victor.stinner@gmail.com> wrote:
Hi,
I wanted to wait a little bit before giving back my feedback on the new workflow. I just attend Brett Canon's talk at the Language Summit. So here are my misc notes on the new workflow.
Is there anyone already working on the workflow who would like to get a grant (money!) from the PSF?
If I want to share a change to review but it must not be merged, would it be possible to prevent merge by mistake? Maybe add [WIP] to the title and modify our bot to mark the "build" as failed? For example, I wrote a patch which depends on a different patch but I didn't want to create a patch serie since it didn't make sense.
AppVeyor is now quite stable. I fixed a few unstable tests. I suggest to mark this CI as mandatory. The CI is as fast or even faster than Travis CI! But it seems like it accepts less jobs in parallel :-/ It might be an issue if we get a huge number of PR in a short period (ex: during an event like Pycon US).
Currently, cherry-picker works a single step. It would be nice to have at least 2 steps: first cherry-pick locally, then allow to review the patch locally and run some specific tests, and then send the PR. By the way, instead of only using the sha1 to build the branch name, it would be nice to add also the bpo number.
I suggest to track backports at bpo since an issue tracks all PR and it seems like most core developers want to put most informations in the issue rather than GitHub.
Find a way to keep the Code coverage job, but don't mark the PR as failed if the coverage is considered as "failed"? It's strange to see a long list of PR with the red sign (failed) whereas Travis and AppVeyor passed.
I have a lot of other minor remarks, but I prefer to stop here ;-)
Victor
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On Wed, 17 May 2017 11:35:29 -0700, Mariatta Wijaya <mariatta.wijaya@gmail.com> wrote:
It's possible, but remember not all PRs have bpo-issue, eg those with trivial label. In that case, what should the backport branch be? So we might end up with two backport branch name patterns, eg
backport-bpo-NNNN-andbackport-sha1for the trivial PRs ?
I don't think it is a problem for the bpo issue number to be missing if there isn't one. (I presume you meant 'backport-bpo-NNNN-sha1` for the BPO alternative.) But how abut using the github PR number in that case?
--David
I don't think it is a problem for the bpo issue number to be missing if
there isn't one. (I presume you meant 'backport-bpo-NNNN-sha1` for the
BPO alternative.) But how abut using the github PR number in that case?
Sure, provided the commit hash, we can get the bpo number and GH pr number by doing:
git log -n 1 --pretty=format:%B <commit_hash>
The question is: since backport branch is temporary and gets deleted once PR is created, is this even important? If so, please open an issue in core-workflow :)
I'm actually not even sure if I should bother with improving cherry_picker.py now knowing a bot is coming soon.
- If I want to share a change to review but it must not be merged, would it be possible to prevent merge by mistake? Maybe add [WIP] to the title and modify our bot to mark the "build" as failed? For example, I wrote a patch which depends on a different patch but I didn't want to create a patch serie since it didn't make sense.
One thing we can do is to apply "don't merge" label, and it can be a status check for bedevere. This is just my suggestion :) bedevere isn't doing this yet. There was talk yesterday about having a bot that can apply labels. Maybe this can be part of that.
- I suggest to track backports at bpo since an issue tracks all PR and
it seems like most core developers want to put most informations in the issue rather than GitHub.
Hmm, what do you mean by tracking backports at bpo? We already have the
needs backport status in bpo.
Mariatta Wijaya
On Wed, May 17, 2017 at 3:59 PM, R. David Murray <rdmurray@bitdance.com> wrote:
On Wed, 17 May 2017 11:35:29 -0700, Mariatta Wijaya < mariatta.wijaya@gmail.com> wrote:
It's possible, but remember not all PRs have bpo-issue, eg those with trivial label. In that case, what should the backport branch be? So we might end up with two backport branch name patterns, eg
backport-bpo-NNNN-andbackport-sha1for the trivial PRs ?I don't think it is a problem for the bpo issue number to be missing if there isn't one. (I presume you meant 'backport-bpo-NNNN-sha1` for the BPO alternative.) But how abut using the github PR number in that case?
--David
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
2017-05-18 9:01 GMT-07:00 Mariatta Wijaya <mariatta.wijaya@gmail.com>:
The question is: since backport branch is temporary and gets deleted once PR is created, is this even important?
I stopped using cherry-picker.py to use a simple shell script for the last step, create a PR from a local branch, because I had too many branches with "random" names. I also like keeping local branches until they are merged.
My shell script ~/bin/gh_pr.sh:
set -e -x if [[ "$1" = "-f" ]]; then force=-f else force= fi local_branch=$(git name-rev --name-only HEAD) ref_branch=$(basename $(pwd)) echo "branches: $local_branch -> $ref_branch" project=cpython git push haypo HEAD $force URL="https://github.com/python/$project/compare/$ref_branch...haypo:$local_branch..." python3 -m webbrowser $URL
If so, please open an issue in core-workflow :)
I will retry cherry-picker.py and try implement my request :-)
Victor
On Wed, 17 May 2017 at 11:27 Victor Stinner <victor.stinner@gmail.com> wrote:
Hi,
I wanted to wait a little bit before giving back my feedback on the new workflow. I just attend Brett Canon's talk at the Language Summit. So here are my misc notes on the new workflow.
Is there anyone already working on the workflow who would like to get a grant (money!) from the PSF?
If I want to share a change to review but it must not be merged, would it be possible to prevent merge by mistake? Maybe add [WIP] to the title and modify our bot to mark the "build" as failed? For example, I wrote a patch which depends on a different patch but I didn't want to create a patch serie since it didn't make sense.
So in this instance couldn't you just not submit the PR until the dependent PR has been accepted? IOW could you just keep the change in your fork until it's ready for review?
- AppVeyor is now quite stable. I fixed a few unstable tests. I suggest to mark this CI as mandatory. The CI is as fast or even faster than Travis CI! But it seems like it accepts less jobs in parallel :-/ It might be an issue if we get a huge number of PR in a short period (ex: during an event like Pycon US).
https://github.com/python/core-workflow/issues/41 (I also have https://github.com/python/core-workflow/issues/91 for turning on macOS support in Travis to test it out, but this wouldn't be required to pass).
Currently, cherry-picker works a single step. It would be nice to have at least 2 steps: first cherry-pick locally, then allow to review the patch locally and run some specific tests, and then send the PR. By the way, instead of only using the sha1 to build the branch name, it would be nice to add also the bpo number.
I suggest to track backports at bpo since an issue tracks all PR and it seems like most core developers want to put most informations in the issue rather than GitHub.
The problem with that is the backport is about cherry-picking the submitted change, not the issue under discussion so there's a disconnect there. Plus we don't typically remove what versions of Python are affected by an issue as we fix things so this would change how we track to what branch we backport to.
- Find a way to keep the Code coverage job, but don't mark the PR as failed if the coverage is considered as "failed"? It's strange to see a long list of PR with the red sign (failed) whereas Travis and AppVeyor passed.
That is already done for new PRs and should be true for all branches (and if it isn't then https://github.com/python/core-workflow/issues/81 will make that true for all branches).
participants (4)
-
Brett Cannon -
Mariatta Wijaya -
R. David Murray -
Victor Stinner