1228007 - Autoland should only push reviewed commits to an inbound repository

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Dan Minor [:dminor]]

At the moment, we attempt to rebase everything on the tip of the destination repository, but this doesn't work for public commits. We should be making a merge commit for these instead and only rebase draft commits.

Comment 1

[Dan Minor [:dminor]]

As discussed in the MozReview meeting, we actually only want to push reviewed commits to destination repository, so the rebase should exclude any public commits along with any unreviewed commits.

Comment 2

[Dan Minor [:dminor]]

I think it makes sense to only do this for "inbound" repositories - most people would expect to be able to do a try push using unreviewed draft commits. As currently implemented, 'try' pushes don't do any rebasing so we won't hit problems with public changesets.

Comment 3

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: make rewritecommitdescriptions return base node for rebases (bug 1228007) r=gps

autoland: make rewritecommitdescriptions return base node for rebases (bug 1228007) r=gps

This changes rewritecommitdescriptions to return the base node for future
rebases. The base node is the oldest node present in commit descriptions or
the rewritten version of that node if it has a modified description. It is an
error if no nodes are present which match the specified commit descriptions.

Comment 4

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: only push reviewed commits to landing repositories (bug 1228007) r=gps

autoland: only push reviewed commits to landing repositories (bug 1228007) r=gps

If trysyntax is not present, we assume we are landing to an inbound
repository and only land reviewed commits. This changes the rebase command
to specify as a source the oldest reviewed commit.

This also changes hglib to run in non-interative mode and modifies the hg
identify command to specify '-r tip'.

Finally, the REST API is modified to enforce specifying either trysyntax
or commit_descriptions.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8693674 [details]
MozReview Request: autoland: make rewritecommitdescriptions return base node for rebases (bug 1228007) r=gps

https://reviewboard.mozilla.org/r/26523/#review23957

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8693675 [details]
MozReview Request: autoland: only push reviewed commits to landing repositories (bug 1228007) r=gps

https://reviewboard.mozilla.org/r/26525/#review23963

::: autoland/autoland/transplant.py:30
(Diff revision 1)
> -    with hglib.open(get_repo_path(tree)) as client:
> +    configs = ['ui.interactive=False']

I would think ui.interactive should be auto detected as false since a TTY isn't present. If it doesn't work this way, this might be an upstream bug. Or perhaps there is a stdin argument to hglib_open() that needs to be set to /dev/null or None?

::: autoland/autoland/transplant.py:64
(Diff revision 1)
>      cmds = [['rebase', '--abort'],
>              ['update', '--clean'],

Both rebase and update have some wonky default behavior with regards to selecting the appropriate revision to operate on. I insist on being explicit.

::: autoland/autoland/transplant.py:78
(Diff revision 1)
> +            if 'no changes found' in output:

At some point it feels easier to just make individual run_hg() calls instead of shoehorning everything into a list of commands with generic error handling that is prone to false positives.

::: autoland/autoland/transplant.py:100
(Diff revision 1)
>          commit_descriptions_file = tempfile.NamedTemporaryFile()

I'm pretty sure you can use NamedTemporaryFile as a context manager so you can avoid the finally.

::: autoland/autoland/transplant.py:142
(Diff revision 1)
> +    cmds.append(['strip', '--no-backup', '-r', 'draft()'])

draft() -> not public()

There are 3 states: public, draft, and secret. Chances are we'll never see secret. But best to be sure it is handled.

::: autoland/tests/test-post-autoland-job.t:154
(Diff revision 1)
> +  $ mozreview exec autoland hg log /repos/inbound-test-repo/ --template '{rev}:{desc\|firstline}:{phase}\\n'
> +  2:Bug 1 - ?????; r=cthulhu:public
> +  1:Bug 1 - more goodness; r=cthulhu:public
> +  0:Bug 1 - some stuff; r=cthulhu:public

Should we specify the encoding so the Unicode is printed?

::: autoland/tests/test-post-autoland-job.t:165
(Diff revision 1)
> -  2:even better ?????
> +  3:Bug 1 - ?????; r=cthulhu:public

Ditto.

Comment 7

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/26525/#review23963

> I would think ui.interactive should be auto detected as false since a TTY isn't present. If it doesn't work this way, this might be an upstream bug. Or perhaps there is a stdin argument to hglib_open() that needs to be set to /dev/null or None?

It's hard-coded True here [1]. I didn't see any obvious way to override it :/

[1] https://selenic.com/repo/python-hglib/file/ffca01835a7c/hglib/client.py#l47

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8693674 [details]
MozReview Request: autoland: make rewritecommitdescriptions return base node for rebases (bug 1228007) r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26523/diff/1-2/

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8693675 [details]
MozReview Request: autoland: only push reviewed commits to landing repositories (bug 1228007) r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26525/diff/1-2/