Autoland should only push reviewed commits to an inbound repository

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
Summary: Autoland needs to support pushing public commits to a repository → Autoland should only push reviewed commits to a repository
(Assignee)

Updated

3 years ago
Blocks: 1220214
(Assignee)

Updated

3 years ago
Assignee: nobody → dminor
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
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.
Summary: Autoland should only push reviewed commits to a repository → Autoland should only push reviewed commits to an inbound repository
(Assignee)

Comment 3

3 years ago
Created attachment 8693674 [details]
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.
Attachment #8693674 - Flags: review?(gps)
(Assignee)

Comment 4

3 years ago
Created attachment 8693675 [details]
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.
Attachment #8693675 - Flags: review?(gps)

Comment 5

3 years ago
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
Attachment #8693674 - Flags: review?(gps) → review+

Comment 6

3 years ago
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.
Attachment #8693675 - Flags: review?(gps) → review+
(Assignee)

Comment 7

3 years ago
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
(Assignee)

Comment 8

3 years ago
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/
(Assignee)

Comment 9

3 years ago
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/
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.