Closed Bug 1347930 Opened 7 years ago Closed 7 years ago

Given a set of commit patches and metadata, the Review Service should create a BMO patch attachment for each.

Categories

(Conduit :: General, enhancement)

Product:
Conduit
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dkl, Assigned: dkl)

References

(
URL
)

Details

Attachments

(1 file, 3 obsolete files)

bugzilla: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)
59 bytes, text/x-review-board-request
mars
: review+
Details 
SSIA
new python package at commitindex/commitindex/reviews/
- add pytest
- add pytest-mountebank from autoland
- BMO REST request/response sequence for creating attachments
- mountebank fixture for req/resp cycles
- errors cascade, BMO nonsense response returns 502, BMO down returns 504

Acceptance criteria:

* Mountebank BMO server for attachments endpoint
* Tests should provide mock data
Comment on attachment 8848580 [details]
commit-index: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)

https://reviewboard.mozilla.org/r/121486/#review123514

::: commitindex/commitindex/reviews/bugzilla.py:27
(Diff revision 1)
> +    """
> +
> +    def __init__(self, rest_url=None):
> +        self.rest_url = rest_url
> +        self.session = requests.Session()
> +        self.logger = logging.getLogger(__name__)

This should be a module-level variable.  It will greatly simplify the code and is idiomatic Python:

```
log = logging.getLogger(__name__)
```

::: commitindex/commitindex/reviews/bugzilla.py:30
(Diff revision 1)
> +        self.rest_url = rest_url
> +        self.session = requests.Session()
> +        self.logger = logging.getLogger(__name__)
> +
> +    def call(self, method, path, data=None):
> +        """Perform REST API call and decode JSON"""

nit: you probably want proper punctuation in the method docstrings.  It would be consistent with your class and module docstring, which is punctuated, and match the [Google docstring style](https://google.github.io/styleguide/pyguide.html#Comments) we use.

::: commitindex/commitindex/reviews/bugzilla.py:46
(Diff revision 1)
> +                                         headers=headers)
> +
> +        try:
> +            data = json.loads(response.content.decode('utf-8'))
> +        except:
> +            raise BugzillaError(400, "Error decoding JSON data")

What does the value '400' represent?

::: commitindex/commitindex/reviews/bugzilla.py:48
(Diff revision 1)
> +        try:
> +            data = json.loads(response.content.decode('utf-8'))
> +        except:
> +            raise BugzillaError(400, "Error decoding JSON data")
> +
> +        if isinstance(data, dict) and 'error' in data and data['error']:

I don't think the `isintance()` type check and `data['error']` are needed.  `if 'error' in data: ...` should work.

::: commitindex/commitindex/reviews/bugzilla.py:59
(Diff revision 1)
> +            if error.fault_code == 102:
> +                return True
> +        except:
> +            raise BugzillaError(error.fault_code, error.fault_string)

I think what you want here is to remove line 61 and 62, and change the `if` statement on line 59 to:

```
if error.fault_code == 102:
  return True
else:
  raise
```

::: commitindex/commitindex/reviews/bugzilla.py:70
(Diff revision 1)
> +            self.call('GET', '/valid_login?login=' + quote(username) +
> +                      '&api_key=' + quote(api_key))

This looks like something requests.get() would handle under the hood with `requests.get(..., params=dict(login=username, api_key=api_key))` If requests does all the quoting and url string prep work for you, then making the requests.get() and self.call() APIs match might be cleaner.

::: commitindex/commitindex/reviews/bugzilla.py:73
(Diff revision 1)
> +            if error.fault_code == 306:
> +                return False
> +        except:
> +            raise BugzillaError(error.fault_code, error.fault_string)

See comment about `BugzillaError` exception handling above, lines 59-62.

::: commitindex/commitindex/reviews/bugzilla.py:83
(Diff revision 1)
> +        The `flags` parameter is an array of flags to set/update/clear.  This
> +        array matches the Bugzilla flag API:
> +        Setting:
> +            {
> +                'id': flag.id
> +                'name': 'review',
> +                'status': '?',
> +                'requestee': reviewer.email
> +            }
> +        Clearing:
> +            {
> +                'id': flag.id,
> +                'status': 'X'
> +            }

Indentation should match the [Google Python style guide](https://google.github.io/styleguide/pyguide.html#Comments)

::: commitindex/commitindex/reviews/bugzilla.py:105
(Diff revision 1)
> +        except BugzillaError as error:
> +            print('code: %d string: %s' % (error.fault_code, error.fault_string))
> +            return None

I think you should let the exception bubble up instead of handling it.  Let the module caller deal with exceptions.

::: commitindex/commitindex/reviews/bugzilla.py:106
(Diff revision 1)
> +        try:
> +            result = self.call('POST', '/bug/' + quote(str(bug_id)) +
> +                               '/attachment?api_key=' + quote(str(api_key)),
> +                               attach_data)
> +        except BugzillaError as error:
> +            print('code: %d string: %s' % (error.fault_code, error.fault_string))

This should be a `log.warning()`, with a message saying "failed to create attachement: code X string Y"

::: commitindex/tests/test_bmo_attachments.py:5
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +"""
> +Test repository handling logic.

needs updating, looks like a copy-n-paste error

::: commitindex/tests/test_bmo_attachments.py:16
(Diff revision 1)
> +import pytest
> +
> +
> +class FakeBugzilla:
> +    """Setups up the imposter test double emulating Bugzilla"""
> +    def __init__(self, mountebank_client):

lint, should be a blank line here

::: commitindex/tests/test_bmo_attachments.py:23
(Diff revision 1)
> +
> +    @property
> +    def url(self):
> +        """Return fully qualified url for server"""
> +        # Copied from the project docker-compose.yml file.
> +        return 'http://172.17.0.2:' + str(self.mountebank.imposter_port)

This hard-coded IP doesn't look right.  Shouldn't it be referring to the docker-compose internal DNS name of the mountebank service?

::: commitindex/tests/test_bmo_attachments.py:77
(Diff revision 1)
> +
> +@pytest.fixture(scope='session')
> +def mountebank():
> +    """Returns configured Mounteback client instance"""
> +    # The docker-compose internal DNS entry for the mountebank container
> +    mountebank_host = "172.17.0.2"

See comment, line 23

::: commitindex/tests/test_bmo_attachments.py:124
(Diff revision 1)
> +    }
> +
> +    bugzilla.create_attachment(1234)
> +    bug_test = Bugzilla(rest_url=bugzilla.url)
> +    result = bug_test.create_attachment(1234, attach_data, '12345')
> +    assert isinstance(result, int)

This should test for the exact data we expect in the response instead of checking the type.

```
assert result == 12345
```
Attachment #8848580 - Flags: review?(mars) → review-
Comment on attachment 8848580 [details]
commit-index: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)

https://reviewboard.mozilla.org/r/121486/#review123514

> This should be a module-level variable.  It will greatly simplify the code and is idiomatic Python:
> 
> ```
> log = logging.getLogger(__name__)
> ```

(the pattern I see in most python codebases is actually to name it `logger` rather than `log` - I wonder why we have this impression of two different conventions?)
Comment on attachment 8848580 [details]
commit-index: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)

https://reviewboard.mozilla.org/r/121486/#review123514

> (the pattern I see in most python codebases is actually to name it `logger` rather than `log` - I wonder why we have this impression of two different conventions?)

Ah, meant to also say, most of the version-control-tools, and other conduit code is using "logger", so using a global named "logger" is going to be more consistent.
Comment on attachment 8848580 [details]
commit-index: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)

https://reviewboard.mozilla.org/r/121486/#review123888

::: commitindex/commitindex/reviews/bugzilla.py:55
(Diff revision 1)
> +
> +        return data
> +
> +    def is_bug_confidential(self, bug_id):
> +        """Check if bug is confidential"""
> +        self.logger.info('Checking if bug %d is confidential.', bug_id)

All of our logging should be using mozlog format, which should be done with https://pypi.python.org/pypi/mozlogging 

The way you're passing the string here and the first argument will have unexpected results if the logging formatter is switched to the mozlogging one. We can talk about the proper invocations in irc or something (I've yet to properly document them for mozlogging).
Attachment #8848580 - Flags: review-
Comment on attachment 8849240 [details]
Conduit: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)

https://reviewboard.mozilla.org/r/122056/#review124202

The code is looking good, just 3 points in the code where a style change to exception handling needs to happen.

::: commitindex/commitindex/reviews/bugzilla.py:96
(Diff revision 2)
>          try:
>              self.call('GET', '/bug/' + quote(str(bug_id)))
>          except BugzillaError as error:
>              if error.fault_code == 102:
>                  return True
> -        except:
> +            raise BugzillaError(error.fault_string, error.fault_code)

This can be changed to re-raise the already-caught BugzillaError with just the `raise` keyword by itself (see the last example of [this section](https://docs.python.org/3/tutorial/errors.html#raising-exceptions) of the Python docs for an example):

```
if error.fault_code == 102:
    return True
raise
```

::: commitindex/commitindex/reviews/bugzilla.py:126
(Diff revision 2)
>              self.call('GET', '/valid_login?login=' + quote(username) +
>                        '&api_key=' + quote(api_key))
>          except BugzillaError as error:
>              if error.fault_code == 306:
>                  return False
> -        except:
> +            raise BugzillaError(error.fault_string, error.fault_code)

This can be changed to re-raise the already-caught BugzillaError with just the `raise` keyword by itself (see the last example of [this section](https://docs.python.org/3/tutorial/errors.html#raising-exceptions) of the Python docs for an example).

::: commitindex/commitindex/reviews/bugzilla.py:157
(Diff revision 2)
> -            return None
> +                {
> +                    'msg': error.fault_string,
> +                    'code': error.fault_code
> +                }, 'app.warning'
> +            )
> +            raise BugzillaError(error.fault_string, error.fault_code)

This can be changed to re-raise the already-caught BugzillaError with just the `raise` keyword by itself (see the last example of [this section](https://docs.python.org/3/tutorial/errors.html#raising-exceptions) of the Python docs for an example).
Attachment #8849240 - Flags: review?(mars) → review-
Attachment #8848580 - Attachment is obsolete: true
Attachment #8849240 - Attachment is obsolete: true
Attachment #8849559 - Attachment is obsolete: true
Attachment #8849559 - Flags: review?(mars)
Comment on attachment 8849687 [details]
bugzilla: bugzilla.py module for accessing BMO's API to add attachments (bug 1347930)

https://reviewboard.mozilla.org/r/122464/#review125068

r+, two nits that I think slipped in while moving around patches.

::: commitindex/commitindex/reviews/bugzilla.py:32
(Diff revision 1)
> +    def __init__(self, rest_url=None):
> +        self.rest_url = rest_url
> +        self.session = requests.Session()
> +
> +    def call(self, method, path, api_key=None, data=None):
> +        """Perform REST API call and decode JSON.i

Typo, line ends with '.i'

::: commitindex/commitindex/reviews/bugzilla.py:76
(Diff revision 1)
> +        try:
> +            data = json.loads(response.content.decode('utf-8'))
> +        except:
> +            raise BugzillaError(400, "Error decoding JSON data")
> +
> +        if isinstance(data, dict) and 'error' in data:

I think this got reverted between patches.  In a previous revision it was:

```
if 'error' in data:
  raise BugzillaError(...)
```
Attachment #8849687 - Flags: review?(mars) → review+
Pushed by mfogels@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/9990a0f8a04e
bugzilla: bugzilla.py module for accessing BMO's API to add attachments r=mars
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: