Closed Bug 1368516 Opened 7 years ago Closed 7 years ago

Add the ability to land patches

Categories

(Conduit Graveyard :: Transplant, enhancement)

Product:
Conduit Graveyard
Component:
Transplant
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mars, Assigned: glob)

References

(
URL
)

Details

Attachments

(13 files, 7 obsolete files)

autoland: refactor request validation (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add request parsing for patch based requests (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add bucket configuration and whitelisting to patch based requests (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: accept and store patch based requests (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: standardise current rev determination in tests (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add tests for patch based requests (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: fix error handling in run_hg (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: refactor mozreview transplant into subclass (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add landing of patch based requests (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add boto3 and friends (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: support landing of s3 hosted patches (bug 1368516)
59 bytes, text/x-review-board-request
gps
: review+
smacleod
: review+
Details
autoland: rework pingback config and code (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add lando pingback support (bug 1368516)
59 bytes, text/x-review-board-request
smacleod
: review+
Details 
Card bears the burden of figuring out how to add and deploy one fully-tested line of code to the vct/autoland/ project code base.

**AC**

- HTTP POST to transplant service endpoint returns HTTP 202
- no job created, but request is placed in job queue
- topic branch? feature flag?
- validation TBD
- auth TBD
- testing TBD
as this is autoland work, moving into the autoland component.
Component: General → Autoland
Product: Conduit → MozReview
Summary: Using the transplant service I can queue a patch from a phabricator revision → Add the ability to land patches from phabricator
Comment on attachment 8872450 [details]
autoland: add Revision API endpoint (bug 1368516)

https://reviewboard.mozilla.org/r/143944/#review147732

::: autoland/autoland/autoland_rest.py:203
(Diff revision 1)
>          return jsonify(status)
>  
>      abort(404)
>  
>  
> +@app.route('/revisions', methods=['POST'])

'revisions' isn't a clear name.
please rename to '/autoland-phabricator'

::: autoland/autoland/autoland_rest.py:205
(Diff revision 1)
>      abort(404)
>  
>  
> +@app.route('/revisions', methods=['POST'])
> +def revisions():
> +    """Resource representing Phabricator Revisions."""

this needs docs, authentication, logging, and input validation
Attachment #8872450 - Flags: review?(glob) → review-
this doesn't appear to be in a state ready to land; i'm going to clear the review flags to remove from my radar.
please re-request review once it's feature complete.
Attachment #8872446 - Flags: review?(glob)
Attachment #8872447 - Flags: review?(glob)
Attachment #8872448 - Flags: review?(glob)
Attachment #8872449 - Flags: review?(glob)
Comment on attachment 8872446 [details]
autoland: configure YAPF for autoland code (bug 1368516)

https://reviewboard.mozilla.org/r/143936/#review148336

reformatting autoland's code is outside the scope of this change.
Attachment #8872446 - Flags: review-
Attachment #8872446 - Attachment is obsolete: true
Attachment #8872447 - Attachment is obsolete: true
Attachment #8872448 - Attachment is obsolete: true
Attachment #8872449 - Attachment is obsolete: true
Attachment #8872450 - Attachment is obsolete: true
Attachment #8873255 - Flags: review?(glob)
Attachment #8873256 - Flags: review?(glob)
Assignee: nobody → glob
Summary: Add the ability to land patches from phabricator → Add the ability to land patches
Attachment #8873255 - Attachment is obsolete: true
Attachment #8873256 - Attachment is obsolete: true
Comment on attachment 8898131 [details]
autoland: refactor request validation (bug 1368516)

https://reviewboard.mozilla.org/r/169478/#review175408
Attachment #8898131 - Flags: review+
Comment on attachment 8898132 [details]
autoland: add request parsing for patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169480/#review175410

I'm going to gently push back on this and suggest that discrete endpoints for discrete actions is better than shoehorning everything into a single endpoint using a myriad of "schemas" for the request payload. A rebuttal to that is it makes clients slightly harder to write.

Another idea would be to require an explicit ``action`` key or something that denotes the requested operation so there is no ambiguity or sniffing of fields to imply it.

::: autoland/autoland/autoland_rest.py:78
(Diff revision 1)
>  
>  
>  def validate_request(request):
>      if request.json is None:
> -        raise ValueError('Bad request: missing json')
> +        raise ValueError('missing json')
> +    json = request.json

Best to not shadow the name of a popular built-in module.

::: autoland/autoland/autoland_rest.py:80
(Diff revision 1)
> -    for field in ['ldap_username', 'tree', 'rev', 'destination',
> -                  'pingback_url']:
> +    required = ['ldap_username', 'tree', 'rev', 'pingback_url', 'destination']
> +    optional = []

Feels like these should be sets.

::: autoland/autoland/autoland_rest.py:103
(Diff revision 1)
> +    for field in required:
> +        if field not in json:
> +            raise ValueError('missing required field: %s' % field)

If you change this to sets, you can do something like:

```
missing = required - set(json.keys())
if missing:
    raise ValueError('missing required fields: %s' % ', '.join(sorted(missing)))
```

This also has the advantage of printing all of the missing fields instead of the first one you encounter. Descriptive error messages like this are super useful for people writing clients.
Comment on attachment 8898133 [details]
autoland: add bucket configuration and whitelisting to patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169482/#review175422
Attachment #8898133 - Flags: review+
Comment on attachment 8898134 [details]
autoland: accept and store patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169484/#review175424
Attachment #8898134 - Flags: review+
Comment on attachment 8898135 [details]
autoland: standardise current rev determination in tests (bug 1368516)

https://reviewboard.mozilla.org/r/169486/#review175426

These should ideally be using full nodes instead of short ones. But whatevs.
Attachment #8898135 - Flags: review+
Comment on attachment 8898137 [details]
autoland: fix error handling in run_hg (bug 1368516)

https://reviewboard.mozilla.org/r/169490/#review175432
Attachment #8898137 - Flags: review+
Comment on attachment 8898140 [details]
autoland: add boto3 and friends (bug 1368516)

https://reviewboard.mozilla.org/r/169496/#review175434

::: test-requirements.txt:28
(Diff revision 1)
> +boto3==1.4.1 \
> +    --hash=sha256:0d5e1d546e3974437f3468ea96abac95ba99753830138c3fb14718c7fdf6121e
> +
> +botocore==1.4.78 \
> +    --hash=sha256:ec05f96c71a7eb97c60ba00fb07879e45fad985736782849abf7afc2d5e960c7

I'd strongly prefer to not update this file and move away from the monolithic virtualenv. Is it too much scope bloat to create a standalone environment for autoland development?
Applying raw patches via `hg import` scares me because it can fail in ways that `hg unbundle` and `hg pull` can't. Here are a few of them:

* Mercurial encoding settings can affect the byte sequence in the author name, commit message, and even the diff content
* Copy and rename metadata may be lost if patch producer isn't using proper settings
* Mercurial may make assumptions about missing data
* Some paths cannot be reliably represented by the patch format (this is rare and also applies to Git)
* `hg import` does not engage the same merge machinery that `hg rebase` does. Pretty much any conflicts will result in failure, even if they could be merged normally.

In short, applying raw patches isn't robust. And the responsibility is on the patch producer to do the right thing. Bundles, by contrast, are much more robust. I would strongly prefer we use bundles as our interchange format instead of patch files. Where is the requirement to use patch files coming from?
Comment on attachment 8898140 [details]
autoland: add boto3 and friends (bug 1368516)

https://reviewboard.mozilla.org/r/169496/#review175434

> I'd strongly prefer to not update this file and move away from the monolithic virtualenv. Is it too much scope bloat to create a standalone environment for autoland development?

yes, creating a standalone env for autoland dev is out of scope here.
Comment on attachment 8898132 [details]
autoland: add request parsing for patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169480/#review175410

(In reply to Gregory Szorc [:gps] from comment #26)
> I'm going to gently push back on this and suggest that discrete endpoints
> for discrete actions is better than shoehorning everything into a single
> endpoint using a myriad of "schemas" for the request payload. A rebuttal to
> that is it makes clients slightly harder to write.

i agree, but the plan for this change is to keep the changes to all parties to a minimum.  we can focus on doing things correctly during the autoland-transplant rewrite, which imho needs to happen this year.
(In reply to Gregory Szorc [:gps] from comment #32)
> Where is the requirement to use patch files coming from?

it boils down to phabricator working with, and generating diffs.


> Copy and rename metadata may be lost if patch producer isn't using proper settings

ah, i should probably use import's --similarity switch.

> Pretty much any conflicts will result in failure, even if they could be merged normally

there is some fuzziness with import, but yes, this could be a problem if import isn't as smart as rebase.


smacleod's work to move to a bundle based systems for review requests should help here (eg. submit the review bundle to transplant instead of a diff from phabricator) but we can't block waiting on that.
Comment on attachment 8898131 [details]
autoland: refactor request validation (bug 1368516)

https://reviewboard.mozilla.org/r/169478/#review181850
Attachment #8898131 - Flags: review?(smacleod) → review+
Comment on attachment 8898132 [details]
autoland: add request parsing for patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169480/#review181854

Looks good for the most part, just r- so I can take another peak at the different cases on update.

::: autoland/autoland/autoland_rest.py:86
(Diff revision 1)
> -            raise ValueError('missing json field: %s' % field)
>  
> -    if not check_pingback_url(request.json['pingback_url']):
> -        raise ValueError('bad pingback_url')
> +    is_try = 'trysyntax' in json
> +    is_patch = 'patch_url' in json
>  
> -    if not (request.json.get('trysyntax') or
> +    if (not is_patch) and not('trysyntax' in json

Missing Space after `not`

::: autoland/autoland/autoland_rest.py:93
(Diff revision 1)
>          raise ValueError('one of trysyntax or commit_descriptions must be '
> -                         'specified.')
> +                         'specified')
> +
> +    if is_try:
> +        required.append('trysyntax')
> +        if is_patch:

Lando will not send a patch with try syntax, or shouldn't, since we won't know where to apply the patch (what base commit to select).

I think things would be a lot clearer here if each of the 3 valid cases were checked explicitly and then their entire fieldset is declared.
Attachment #8898132 - Flags: review?(smacleod) → review-
Comment on attachment 8898133 [details]
autoland: add bucket configuration and whitelisting to patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169482/#review181862
Attachment #8898133 - Flags: review?(smacleod) → review+
Comment on attachment 8898134 [details]
autoland: accept and store patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169484/#review181864
Attachment #8898134 - Flags: review?(smacleod) → review+
Comment on attachment 8898135 [details]
autoland: standardise current rev determination in tests (bug 1368516)

https://reviewboard.mozilla.org/r/169486/#review181866
Attachment #8898135 - Flags: review?(smacleod) → review+
Comment on attachment 8898136 [details]
autoland: add tests for patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169488/#review181868
Attachment #8898136 - Flags: review?(smacleod) → review+
Comment on attachment 8898137 [details]
autoland: fix error handling in run_hg (bug 1368516)

https://reviewboard.mozilla.org/r/169490/#review181870
Attachment #8898137 - Flags: review?(smacleod) → review+
Comment on attachment 8898138 [details]
autoland: refactor mozreview transplant into subclass (bug 1368516)

https://reviewboard.mozilla.org/r/169492/#review181874
Attachment #8898138 - Flags: review?(smacleod) → review+
Comment on attachment 8898139 [details]
autoland: add landing of patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169494/#review181876

::: autoland/autoland/transplant.py:274
(Diff revision 1)
> +    def __init__(self, tree, destination, rev, patch_url):
> +        self.patch_url = patch_url
> +
> +        super(PatchTransplant, self).__init__(tree, destination, rev)
> +
> +    def apply_changes(self, remote_tip):

We're passing in `remote_tip` but not using it. Do we need to make sure we're updating to it before calling import?
Attachment #8898139 - Flags: review?(smacleod) → review-
Comment on attachment 8898140 [details]
autoland: add boto3 and friends (bug 1368516)

https://reviewboard.mozilla.org/r/169496/#review181878
Attachment #8898140 - Flags: review?(smacleod) → review+
Comment on attachment 8898141 [details]
autoland: support landing of s3 hosted patches (bug 1368516)

https://reviewboard.mozilla.org/r/169498/#review181880

Overall looks pretty good to me. going to r- to give a final pass when this is updated.

::: autoland/autoland/transplant.py:297
(Diff revision 1)
> +            temp_file = self._download_from_url()
> +
> +        try:
> +            # Apply the patch
> +            logger.debug(self.run_hg(['import', temp_file]))
> +        finally:

The try for this finally block should include the downloading of the temp file (which includes its creation), so that if something in those function calls fail we still unlink.

::: autoland/autoland/transplant.py:320
(Diff revision 1)
> +        bucket_config = buckets_config[bucket]
> +
> +        if ('aws_access_key_id' not in bucket_config or
> +                'aws_secret_access_key' not in bucket_config):
> +            logging.error('bucket "%s" is missing aws_access_key_id or '
> +                          'aws_secret_access_key' % bucket)

We're continuing, shouldn't we throw an exception?

::: autoland/autoland/transplant.py:342
(Diff revision 1)
> +            if error_code == 403:
> +                raise Exception('unable to download %s: permission denied'
> +                                % self.patch_url)
> +            raise
> +
> +        return temp_file.name

Ah okay, now I see, you won't be able to return the temp file name if something fails here, so the caller can't clean it up.
Attachment #8898141 - Flags: review?(smacleod) → review-
Comment on attachment 8898142 [details]
autoland: rework pingback config and code (bug 1368516)

https://reviewboard.mozilla.org/r/169500/#review181894

::: autoland/autoland/autoland.py:277
(Diff revision 1)
> +            # and testing.
> +            pingback_url = None
> +
> +        else:
> +            if hostname not in config.get('pingback', {}):
> +                logging.warning('ignoring pingback to %s: unconfigured'

This feels like it should be a `logging.error` instead of `warning`.
Attachment #8898142 - Flags: review?(smacleod) → review+
Comment on attachment 8898143 [details]
autoland: add lando pingback support  (bug 1368516)

https://reviewboard.mozilla.org/r/169502/#review181902
Attachment #8898143 - Flags: review?(smacleod) → review+
Comment on attachment 8898132 [details]
autoland: add request parsing for patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169480/#review183300

Just two small nits, r+ with these fixed.

::: autoland/autoland/autoland_rest.py:81
(Diff revisions 1 - 2)
>      if request.json is None:
>          raise ValueError('missing json')
> -    json = request.json
> +    request_json = request.json
>  
> -    required = ['ldap_username', 'tree', 'rev', 'pingback_url', 'destination']
> -    optional = []
> +    required = {'ldap_username', 'tree', 'rev', 'pingback_url', 'destination'}
> +    optional = set([])

`optional = set()`, no need for the list.

::: autoland/autoland/autoland_rest.py:113
(Diff revision 2)
> +
> +    request_fields = set(request_json.keys())
> +
> +    missing = required - request_fields
> +    if missing:
> +        raise ValueError('missing required field%ss: %s' % (

you have `field%ss:`, should it not be `field%s:`?
Attachment #8898132 - Flags: review?(smacleod) → review+
Comment on attachment 8898139 [details]
autoland: add landing of patch based requests (bug 1368516)

https://reviewboard.mozilla.org/r/169494/#review183322

two nits, r+ after fixed.

::: autoland/autoland/autoland.py:99
(Diff revisions 1 - 2)
> -        patch_url = request.get('patch_url', '').encode('ascii')
> +        patch_urls = map(lambda u: u.encode('ascii'),
> +                         request.get('patch_urls', []))

Generally more pythonic to go with a list comprehension: `[u.encode('ascii') for u in request.get('patch_urls', [])]`

::: autoland/autoland/transplant.py:283
(Diff revisions 1 - 2)
> +            if patch_url.startswith('s3://'):
> -            # Download patch to temp file and import
> +                # Download patch to temp file and import
> -            raise Exception('importing patches from s3 not implemented')
> +                raise Exception('importing patches from s3 not implemented')
>  
> -        else:
> +            else:
> -            output = self.run_hg(['import', self.patch_url])
> +                self.run_hg(['checkout', remote_tip])

`update` is the actual command, `checkout` is an alias for convenience, I'd rather we use the real command name.
Attachment #8898139 - Flags: review?(smacleod) → review+
Comment on attachment 8898141 [details]
autoland: support landing of s3 hosted patches (bug 1368516)

https://reviewboard.mozilla.org/r/169498/#review183334

lgtm

::: autoland/autoland/transplant.py:284
(Diff revision 2)
>          super(PatchTransplant, self).__init__(tree, destination, rev)
>  
>      def apply_changes(self, remote_tip):
>          assert self.patch_urls, 'patch_urls not provided'
>  
> +        self.run_hg(['checkout', remote_tip])

`update` vs `checkout` again.
Attachment #8898141 - Flags: review?(smacleod) → review+
Comment on attachment 8898141 [details]
autoland: support landing of s3 hosted patches (bug 1368516)

https://reviewboard.mozilla.org/r/169498/#review184122

Needs at least a question answered about working directory cleanliness. The download and temp file changes could be deferred if wanted.

::: autoland/autoland/transplant.py:284
(Diff revision 3)
>          super(PatchTransplant, self).__init__(tree, destination, rev)
>  
>      def apply_changes(self, remote_tip):
>          assert self.patch_urls, 'patch_urls not provided'
>  
> +        self.run_hg(['update', remote_tip])

Do we verify a clean working directory anywhere before attempting this operation? If not, the working directory state could be non-deterministic after this operation.

We can verify a clean working directory by doing something like `hg status --modified --added --removed --deleted --unknown --ignored` and verifying the output is empty.

::: autoland/autoland/transplant.py:287
(Diff revision 3)
> +            temp_file = tempfile.NamedTemporaryFile(delete=False)
> +            temp_file.close()
> +            temp_filename = temp_file.name

Strictly speaking, there are race conditions here. It would be preferable to download to a buffer (likely an ``io.BytesIO``) then write out the file only if needed.

But this code doesn't even need to involve a file! `hg import -` will import from stdin. So it should be possible to pipe the downloaded file content to the `hg import` invocation. Although that may enough extra plumbing code that a temporary file is just easier. That's fine.

::: autoland/autoland/transplant.py:312
(Diff revision 3)
> +    def _download_from_s3(filename, patch_url):
> +        # Download from s3 url specified in self.patch_url to the specified

I'd change this and the function below to either return an ``io.BytesIO`` or a raw ``str``/``bytes`` of the downloaded content. I don't think any inputs will be large enough that buffering will be a problem.
Attachment #8898141 - Flags: review?(gps) → review-
Comment on attachment 8898141 [details]
autoland: support landing of s3 hosted patches (bug 1368516)

https://reviewboard.mozilla.org/r/169498/#review184122

> Do we verify a clean working directory anywhere before attempting this operation? If not, the working directory state could be non-deterministic after this operation.
> 
> We can verify a clean working directory by doing something like `hg status --modified --added --removed --deleted --unknown --ignored` and verifying the output is empty.

draft commits are stripped and update is run with --clean, however you're right that we should do more and sanity check to ensure a clean state.  i'll add more sanitisation and a sanity check before that `hg update`.

prod's gecko repo has a ton of .orig files; they shouldn't be harmful but more scrubbing of the repo is definitely required.

> Strictly speaking, there are race conditions here. It would be preferable to download to a buffer (likely an ``io.BytesIO``) then write out the file only if needed.
> 
> But this code doesn't even need to involve a file! `hg import -` will import from stdin. So it should be possible to pipe the downloaded file content to the `hg import` invocation. Although that may enough extra plumbing code that a temporary file is just easier. That's fine.

excellent point; i'll have a play with this.
Blocks: servo-coland
Comment on attachment 8898141 [details]
autoland: support landing of s3 hosted patches (bug 1368516)

https://reviewboard.mozilla.org/r/169498/#review197288
Attachment #8898141 - Flags: review?(gps) → review+
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/111c55b0d9a1
autoland: refactor request validation r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/63fb803173f7
autoland: add request parsing for patch based requests r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/97f2233a3a71
autoland: add bucket configuration and whitelisting to patch based requests r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6290aebdf58e
autoland: accept and store patch based requests r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f1fe627708cd
autoland: standardise current rev determination in tests r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d9112a57b18d
autoland: add tests for patch based requests r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a1b856bd7c0b
autoland: fix error handling in run_hg r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3187f8b05fef
autoland: refactor mozreview transplant into subclass r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f40c8ca71128
autoland: add landing of patch based requests r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4c8e1cb7f089
autoland: add boto3 and friends r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/23617d507a20
autoland: support landing of s3 hosted patches r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/036e7cb57754
autoland: rework pingback config and code r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f64eca932444
autoland: add lando pingback support r=smacleod
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/bcf6e3b0c9a1
autoland: instantiate byteio object instead
Product: MozReview → Conduit
Product: Conduit → Conduit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: