mots: create initial prototype (bug 1733956) #1

zzzeid · 2021-10-04T14:28:01Z

A helper Python library to facilitate in-tree module ownership system integration.

This includes: - makefile to assist with development environment - support for basic commands to parse and validate mots.yaml - support for modules and submodules - support for a directory and searchable index - cli interface - tests TODOs: - add more environment-specific variables to be by mots internally - add circleci and readthedocs configuration - improve documentation - improve readability of fixtures

NOTE: could also break this into different branches and facilitate code review that way. Includes: - PMO and BMO thin clients that were used to parse and validate users - bug fix to return rejected paths when querying - cli command to add module (already obsolete?) and helper function - bug fix when determining repo path in config (was fetching bad index) - Directory class improvements, including: - new People and Person helper classes - load people into Directory.people - serialize people and rewrite yaml when needed - validate people against BMO user IDs - improvement to yaml config format including using named anchors - improved logging - helper function to parse user string (used in import, but reusable) - fixed serialization of modules - fixed references to people in yaml (uses nick-named anchors now) TODO: - improve PMO/BMO client implementation - clean up helper functions - refactor/clean up module.py to have less repetition in submodules - move `extract_people` to utils - add more tests and mock BMO client

- fix bug with BMO token input - fix bug with `module add` command

- add readthedocs config - update directory query response to use QueryResponse class - conftest bug fix

- update QueryResult.__add__ to return a QueryResult object - minor fixes to documentation - add test for QueryResult - update command line usage in docs - temporarily move readme to readthedocs link

- fix QueryResult.rejected_paths sum to find intersection only - add wiki export functionality (bug 1743476)

- add hashes to requirements file + make command

- use isoformat for timestamps - add parse_real_name method to parse bmo data into name and info

- fix typo in command help - fix query command output - fix FileConfig default argument when loading config

src/mots/bmo.py

src/mots/directory.py

cgsheeh

This was my first pass. I'll jump in a bit deeper tomorrow.

src/mots/cli.py

src/mots/directory.py

README.md

src/mots/bmo.py

src/mots/cli.py

src/mots/directory.py

zzzeid · 2022-01-19T21:00:55Z

This was my first pass. I'll jump in a bit deeper tomorrow.

Thanks for the feedback @cgsheeh -- definitely a few remnants from the early approaches left in here, which you've commented on (e.g. the use of input and some of the issues in cli as well as generic exceptions.) I'll address the feedback tomorrow but do expect a few more rough edges.

Makefile

documentation/conf.py

mitchhentges · 2022-01-19T23:50:56Z

requirements.in

@@ -0,0 +1,11 @@
+black==21.7b0


Nit: consider poetry for having a cross-platform lockfile, though this is good as well.

Going to hold off on that for now, but will explore this idea for the future.

Quoting your question from Slack

Specifically for Poetry, I'm not familiar with it, but I don't see any existing issues with the way I've handled requirements that I need to resolve.

pip-compile doesn't support cross-platform lockfiles.
So, if a dependency has a platform- or python-version-specific dependency (e.g. import_metadata; python_version < 3.7), the pip-compile generates the requirements.txt according to the current system, rather than processing and "carrying forward" conditional dependencies into the generated lockfile.

This won't be an issue if

Your lockfile is only used by a specific Python version on a specific platform

Or, if you don't have any conditional transitive dependencies.

One way you might see this manifest is if:

You generate requirements.txt with Python 3.9 on macOS

In Linux CI, you get a " requirement is not pinned" error.

TL;DR: I'm ok with leaving this as-is, but I want to make sure that I'm properly communicating why pip-compile isn't sufficient for the long-term.

TL;DR: I'm ok with leaving this as-is, but I want to make sure that I'm properly communicating why pip-compile isn't sufficient for the long-term.

Noted, and I am aware of the limitations of pip-compile that you mentioned (MozPhab has a similar but slightly more complex situation, and there I've solved that with simple docker containers to generate those files for each platform). I don't necessarily see this issue as needing another library and additional config files for a long term solution, but it's certainly something to consider.

Keep in mind that the requirements.txt file is mostly needed for CircleCI purposes, and also for local development. mots itself only has two requirements which are included in setup.cfg and does not require any lock files or anything like that. For CircleCI we always know ahead of time which platform we'll be running tests on.

Another minor comment here is that the only requirement that is python-version-specific for packaging mots, is importlib-resources. It's definitely not an unimportant part of the code (since it will be used when exporting to rst) but I'd like to find a cleaner solution to fetch templates in the future, so this conditional dependency may be removed in the future.

Filed a bug for this. The requirements files will need to be fixed before merging/deploying this since tests will actually fail on Python 3.7 as things stand now.

src/mots/bmo.py

src/mots/config.py

src/mots/directory.py

src/mots/cli.py

mitchhentges · 2022-01-20T00:40:48Z

Pretty exciting so far, keep it up :)

cgsheeh

I saw a few more small nits/things that stood out to me. I'm going to hold off on a more thorough review until some of the deprecated code is removed and we have our meeting next week. :)

src/mots/module.py

Makefile

src/mots/bmo.py

- fix peers attribute on module - fix description attribute on module - return submodules and parent when serializing

cgsheeh

This was a deeper dive. Most of the core logic seems sound to me! 👍

Meta note: please use proper variable names in loops, ie no m, p, sm etc. There were many instances so I stopped commenting on them all. Same with range(len(obj)) vs enumerate.

src/mots/config.py

src/mots/directory.py

src/mots/module.py

setup.cfg

src/mots/directory.py

src/mots/module.py

src/mots/pmo.py

cgsheeh · 2022-01-31T17:00:06Z

src/mots/config.py

+            # File does not exist, create it.
+            now = datetime.now().isoformat()
+            self.config = {
+                "repo": str(Path(self.path).resolve().parts[-2]),


Is -2 specific to ./mots.yaml here? Could a different path be passed that breaks this, ie via the --path CLI argument? Maybe it's fine since the path arg to init is for the repo itself, but just want to put it on your radar.

@zzzeid in Slack you said you wanted to mention something here, what was it? :)

So the answer is yes, it is specific to this path. Since the path to the config file is assumed to be in the top level directory of the repo, this should never cause any problems. It might be good to add some more explicit checks to make sure that mots.yaml is always at the top level, but basically if this is not the case, then other things will break. Agreed that this needs to evolve a little more though -- maybe once I add more support for other source control awareness (e.g. hg files).

Makes sense. I won't block the review on this, but adding a TODO comment here would be nice.

Filed a bug for this.

For readability's sake, this could be better represented as str(Path(self.path).resolve().parent.name)

- add export to rst functionality; deprecate wiki export

- Fixes bug 1750200 (yaml_set_anchor issue)

- Fix `mots module add` command so that it adds bmo_id key - Fix `mots init` command to include "people" key

cgsheeh

Looks good, the main thing remaining here is cleanup of the try/except KeyError block and followup on the -2 thing.

I pulled this PR down and ran the CLI a few times. I noticed a few issues: first the representations printed to the screen are Python text representations (like [defaultdict()...] etc). If we don't want to block this on landing, perhaps we could pprint.pprint instead of print as a quick solution.

Next, when I run mots query I see a Bugzilla error. Is this expected/can we make this better?

❯ BUGZILLA_API_KEY=<redacted> mots query src/
2022-02-03 11:46:17,698 directory  WARNING  No bugzilla data was provided for user 633708
src/:main

There is also a lot of unrelated spew when running mots without specifying the proper subcommand/arguments. See below, where mots --help prints what I would expect for mots, and mots module without a subcommand also prints the help for every command:

❯ mots --help
usage: mots [-h] [--debug] {init,module,validate,clean,query,export} ...

main command line interface for mots

optional arguments:
  -h, --help            show this help message and exit
  --debug               enable debug output

commands:
  {init,module,validate,clean,query,export}
    init                initialize mots configuration in repo
    module              module_operations
    validate            validate mots config for current repo
    clean               clean mots config for current repo
    query               query the module directory
    export              export the module directory
❯ mots
usage: mots [-h] [--debug] {init,module,validate,clean,query,export} ...

main command line interface for mots

optional arguments:
  -h, --help            show this help message and exit
  --debug               enable debug output

commands:
  {init,module,validate,clean,query,export}
    init                initialize mots configuration in repo
    module              module_operations
    validate            validate mots config for current repo
    clean               clean mots config for current repo
    query               query the module directory
    export              export the module directory

usage: mots init [-h] [--path PATH]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo to initialize

usage: mots module [-h] [--path PATH] {list,add,show} ...

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file

module:
  {list,add,show}
    list                list all modules
    add                 add a new module
    show                show a module

usage: mots validate [-h] [--path PATH] [--repo-path REPO_PATH]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file
  --repo-path REPO_PATH, -r REPO_PATH
                        the path of the repo

usage: mots clean [-h] [--path PATH] [--repo-path REPO_PATH]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file
  --repo-path REPO_PATH, -r REPO_PATH
                        the path of the repo

usage: mots query [-h] [--path PATH] paths [paths ...]

positional arguments:
  paths                 a list of paths to query

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file

usage: mots export [-h] [--path PATH] [--format FORMAT]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file
  --format FORMAT, -f FORMAT
                        the format of exported data
❯ mots module
usage: mots [-h] [--debug] {init,module,validate,clean,query,export} ...

main command line interface for mots

optional arguments:
  -h, --help            show this help message and exit
  --debug               enable debug output

commands:
  {init,module,validate,clean,query,export}
    init                initialize mots configuration in repo
    module              module_operations
    validate            validate mots config for current repo
    clean               clean mots config for current repo
    query               query the module directory
    export              export the module directory

usage: mots init [-h] [--path PATH]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo to initialize

usage: mots module [-h] [--path PATH] {list,add,show} ...

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file

module:
  {list,add,show}
    list                list all modules
    add                 add a new module
    show                show a module

usage: mots validate [-h] [--path PATH] [--repo-path REPO_PATH]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file
  --repo-path REPO_PATH, -r REPO_PATH
                        the path of the repo

usage: mots clean [-h] [--path PATH] [--repo-path REPO_PATH]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file
  --repo-path REPO_PATH, -r REPO_PATH
                        the path of the repo

usage: mots query [-h] [--path PATH] paths [paths ...]

positional arguments:
  paths                 a list of paths to query

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file

usage: mots export [-h] [--path PATH] [--format FORMAT]

optional arguments:
  -h, --help            show this help message and exit
  --path PATH, -p PATH  the path of the repo config file
  --format FORMAT, -f FORMAT
                        the format of exported data

mots.yaml

cgsheeh · 2022-02-03T15:14:01Z

src/mots/config.py

+            # File does not exist, create it.
+            now = datetime.now().isoformat()
+            self.config = {
+                "repo": str(Path(self.path).resolve().parts[-2]),


@zzzeid in Slack you said you wanted to mention something here, what was it? :)

src/mots/config.py

src/mots/cli.py

src/mots/config.py

mots.yaml

zzzeid · 2022-02-04T18:00:38Z

Looks good, the main thing remaining here is cleanup of the try/except KeyError block and followup on the -2 thing.

The try/except thing is fixed now.

I pulled this PR down and ran the CLI a few times. I noticed a few issues: first the representations printed to the screen are Python text representations (like [defaultdict()...] etc). If we don't want to block this on landing, perhaps we could pprint.pprint instead of print as a quick solution.

Fixed.

Next, when I run mots query I see a Bugzilla error. Is this expected/can we make this better?

Fixed.

There is also a lot of unrelated spew when running mots without specifying the proper subcommand/arguments. See below, where mots --help prints what I would expect for mots, and mots module without a subcommand also prints the help for every command:

Fixed. There was originally a for loop that iterated over all subparsers and printed the help. This (may have) made sense when there were only 2 commands but now it does not. I think I originally added for debugging, so nice catch!

mitchhentges

Looks pretty good, got a couple nits and a couple needed cleanups (specifically the TODOs).

To make sure this is tracked with the PR: as mentioned on Slack, it may be worth using a single BMO identifier in the mots.yaml, then populating the rest from BMO at output-generation-time. This would allow reducing mots.yaml churn and removing the need for a CLI used by end developers.
However, due to the significant changes required by the endeavour and the stage of this project, it's not a blocking issue - it may be worth reconsidering in the future though.

TL;DR: Nice work, looking forward to deploying this "for real" in-tree :)

.circleci/config.yml

mitchhentges · 2022-02-04T18:48:26Z

requirements.in

@@ -0,0 +1,11 @@
+black==21.7b0


Quoting your question from Slack

Specifically for Poetry, I'm not familiar with it, but I don't see any existing issues with the way I've handled requirements that I need to resolve.

pip-compile doesn't support cross-platform lockfiles.
So, if a dependency has a platform- or python-version-specific dependency (e.g. import_metadata; python_version < 3.7), the pip-compile generates the requirements.txt according to the current system, rather than processing and "carrying forward" conditional dependencies into the generated lockfile.

This won't be an issue if

Your lockfile is only used by a specific Python version on a specific platform

Or, if you don't have any conditional transitive dependencies.

One way you might see this manifest is if:

You generate requirements.txt with Python 3.9 on macOS

In Linux CI, you get a " requirement is not pinned" error.

TL;DR: I'm ok with leaving this as-is, but I want to make sure that I'm properly communicating why pip-compile isn't sufficient for the long-term.

src/mots/bmo.py

mitchhentges · 2022-02-04T18:53:55Z

src/mots/bmo.py

+        self.headers = {"X-BUGZILLA-API-KEY": token, "User-Agent": USER_AGENT}
+        self.base_url = base_url


I'd lean towards marking these variables as private (prefixed with _) as part of this initial patch because:

It's low-risk, since if you miss a migration it'll blow up pretty quick at runtime

Since mots can be used as an embeddable library, it'll be trickier to do internal refactoring without potentially impacting consumers that are accessing "public" member variables.

I agree in this instance. Updated.

mitchhentges · 2022-02-04T19:46:57Z

src/mots/module.py

+
+logger = logging.getLogger(__name__)
+
+print = pprint.PrettyPrinter().pprint


Nit:

from pprint import pprint ... print = pprint

That should be functionally equivalent to pprint.PrettyPrinter().pprint, right?

I suppose we could even do

from pprint import pprint as print

src/mots/module.py

mitchhentges · 2022-02-04T19:49:31Z

src/mots/module.py

+        name: str = None,
+        description: str = None,


Small nit:
Having an Optional[str] has essentially three states:

None

""

"<contents>"

For most intents and purposes, None == "", and having to track both cases for validation/printing/etc can be a pain.
It might be worth considering leaning towards using "" to represent "unset string properties" instead of None to simplify related logic.

Fixed those two, and also the type hints were incorrect for includes and excludes below, which are also fixed now.

mitchhentges · 2022-02-04T19:52:25Z

src/mots/utils.py

+    return [e.strip() for e in user_input if e]
+
+
+def parse_user_string(string):


I don't think this is used

Nice catch. This lingered on from the initial migration of the wiki page; will remove.

mitchhentges · 2022-02-04T19:52:58Z

src/mots/utils.py

+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+"""Utility helper functions."""


This utilities can probably all be moved closer to their associated domains for better discoverability:

generate_machine_readable_name: This might be better being beside the class Module definition.

get_list_input: Chuck this in cli.py IMHO

parse_real_name: This can be beside class Person

I do like being able to separate and test these small methods that do not rely on any external classes in their own module (i.e. utils). I can see why they could be moved to a different module if they had any dependencies, but they're basically text parsers that may be used by any other module - thus I think they're fine to stay here.

zzzeid · 2022-02-04T21:20:02Z

.circleci/config.yml

+            set -e
+            sudo apt-get install python3-venv
+            mkdir -p /tmp/test-reports
+            make dev-env PY=python3.9


Suggested change

make dev-env PY=python3.9

make dev-env

- generate separate requirement files for each supported python - update requirements to latest versions where applicable - update make commands to auto-generate requirements and environments

cgsheeh

Well done!

requirements/base.in

Makefile

mitchhentges

Nice work! 👏

zzzeid and others added 11 commits October 4, 2021 10:23

circleci: add circleci configuration (bug-1740083)

3ebc629

Additional improvements

645fb9a

- fix bug with BMO token input - fix bug with `module add` command

ci: add circleci integration (bug 1740083)

26bd5b3

Additional improvements

8e0165d

- add readthedocs config - update directory query response to use QueryResponse class - conftest bug fix

Additional improvements:

16ed62e

- update QueryResult.__add__ to return a QueryResult object - minor fixes to documentation - add test for QueryResult - update command line usage in docs - temporarily move readme to readthedocs link

Additional improvements:

e5c31c6

- fix QueryResult.rejected_paths sum to find intersection only - add wiki export functionality (bug 1743476)

Additional improvements:

9dfac7e

- add hashes to requirements file + make command

Additional improvements:

15373df

- use isoformat for timestamps - add parse_real_name method to parse bmo data into name and info

Small fixes:

ec11d39

- fix typo in command help - fix query command output - fix FileConfig default argument when loading config

zzzeid marked this pull request as ready for review January 19, 2022 17:22

zzzeid requested review from cgsheeh, dklawren and mitchhentges January 19, 2022 17:28

zzzeid commented Jan 19, 2022

View reviewed changes

src/mots/bmo.py Outdated Show resolved Hide resolved

src/mots/bmo.py Outdated Show resolved Hide resolved

src/mots/directory.py Outdated Show resolved Hide resolved

cgsheeh requested changes Jan 19, 2022

View reviewed changes

mitchhentges reviewed Jan 20, 2022

View reviewed changes

cgsheeh reviewed Jan 21, 2022

View reviewed changes

src/mots/module.py Outdated Show resolved Hide resolved

src/mots/module.py Outdated Show resolved Hide resolved

src/mots/module.py Outdated Show resolved Hide resolved

src/mots/module.py Outdated Show resolved Hide resolved

zzzeid commented Jan 24, 2022

View reviewed changes

Makefile Show resolved Hide resolved

zzzeid commented Jan 24, 2022

View reviewed changes

src/mots/bmo.py Outdated Show resolved Hide resolved

zzzeid commented Jan 24, 2022

View reviewed changes

src/mots/bmo.py Outdated Show resolved Hide resolved

zzzeid added 2 commits January 28, 2022 14:12

Small fixes:

2c7b567

- fix peers attribute on module - fix description attribute on module - return submodules and parent when serializing

Code review feedback

fa5d48f

cgsheeh requested changes Jan 31, 2022

View reviewed changes

zzzeid added 3 commits February 2, 2022 09:14

Additional improvements:

c08f758

- add export to rst functionality; deprecate wiki export

Code review feedback + bug fixes

efcb8f5

- Fixes bug 1750200 (yaml_set_anchor issue)

update .gitignore

b8fa8b5

zzzeid added 2 commits February 2, 2022 14:58

Bug fixes + dog fooding

857f82b

- Fix `mots module add` command so that it adds bmo_id key - Fix `mots init` command to include "people" key

Minor change

9c72723

cgsheeh requested changes Feb 3, 2022

View reviewed changes

zzzeid commented Feb 3, 2022

View reviewed changes

mots.yaml Outdated Show resolved Hide resolved

zzzeid added 2 commits February 4, 2022 12:49

code review feedback + other fixes

97cee66

pretty print + remove accidentally added file

a918a37

zzzeid requested review from cgsheeh and mitchhentges February 4, 2022 18:05

Minor changes

38e3490

mitchhentges suggested changes Feb 4, 2022

View reviewed changes

add more circleci environments; code review feedback

fa9258f

zzzeid commented Feb 4, 2022

View reviewed changes

Additional improvements:

271dbbe

- generate separate requirement files for each supported python - update requirements to latest versions where applicable - update make commands to auto-generate requirements and environments

cgsheeh approved these changes Feb 7, 2022

View reviewed changes

requirements/base.in Show resolved Hide resolved

code review feedback + small refactoring

898fa78

zzzeid requested a review from mitchhentges February 7, 2022 20:42

zzzeid commented Feb 7, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

mitchhentges approved these changes Feb 7, 2022

View reviewed changes

zzzeid added 3 commits February 8, 2022 08:30

remove extra line in Makefile

5ef6ce2

update copyright

56d380f

develop -> latest in README docs reference

fb36034

zzzeid merged commit 2b25111 into main Feb 8, 2022

zzzeid deleted the zeid/bug-1733956-create-initial-prototype branch February 8, 2022 13:51

		self.headers = {"X-BUGZILLA-API-KEY": token, "User-Agent": USER_AGENT}
		self.base_url = base_url


		logger = logging.getLogger(__name__)

		print = pprint.PrettyPrinter().pprint

		return [e.strip() for e in user_input if e]


		def parse_user_string(string):

mots: create initial prototype (bug 1733956) #1

mots: create initial prototype (bug 1733956) #1

Conversation

zzzeid commented Oct 4, 2021 • edited Loading

cgsheeh left a comment

Choose a reason for hiding this comment

zzzeid commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchhentges commented Jan 20, 2022

cgsheeh left a comment

Choose a reason for hiding this comment

cgsheeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgsheeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzzeid commented Feb 4, 2022

mitchhentges left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgsheeh left a comment

Choose a reason for hiding this comment

mitchhentges left a comment

Choose a reason for hiding this comment

zzzeid commented Oct 4, 2021 •

edited

Loading