Publish fidesops to PyPI #491

PSalant726 · 2022-05-10T22:46:26Z

Purpose

Fidesops is not currently published to PyPI. This makes it difficult to install and use fidesops as a package in other Python projects. As work proceeds on https://github.com/ethyca/fidesops-plus, installing fidesops via git becomes cumbersome, as the enterprise team would like to maintain feature parity by pinning the imported version of fidesops.

Changes

Add missing __init__.py files (12 total)
- And fix all ensuing static analysis errors
Add a GitHub Actions workflow to publish fidesops to PyPI whenever a new tag is created (releases)
- The PYPI_TOKEN secret needs to be configured on this repository
Alphabetize requirements.txt
Add the versioneer dependency
Install versioneer
- And use it in setup.py

Checklist

Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
If docs updated (select one):
- documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
- documentation issue created (tag docs-team to complete issue separately)
Good unit test/integration test coverage
This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #481

There are no version number or constraint changes - only ordering.

PSalant726

The large number of added files is due mostly to the new __init__.py files that were previously missing, and the files added by versioneer.

PSalant726 · 2022-05-10T22:48:08Z

.github/workflows/publish_to_pypi.yml

+on:
+  push:
+    tags:
+      - "*"


There may be a better trigger for this workflow based on the preferred release process.

@PSalant726 check fidesctl we do it in a smarter way for the docs -> https://github.com/ethyca/fides/blob/main/.github/workflows/publish_docs.yaml#L7

Updated. @seanpreston does this now align with the current release process?

.github/workflows/publish_to_pypi.yml

requirements.txt

To automate version numbers

These errors are being surfaced due to the new `__init__.py` files.

ThomasLaPiana · 2022-05-11T18:30:06Z

I highly highly strongly recommend adopting the fidesctl docker setup so that in the production image the package is installed as if it was coming directly from pypi, it will catch so many potential packaging errors (such as no init files)

pyproject.toml

ThomasLaPiana · 2022-05-11T18:38:12Z

in fidesctl we use multi-stage builds for our dockerfile. This means that when we run our tests we use a production image with fidesctl installed from a clean build, instead of symlinked. This makes sure that our tests run against a package that has been installed as if it were a pip install fidesctl.

This is definitely for another issue, but is a best practice that should be done in a follow-up to this one

src/fidesops/api/v1/endpoints/privacy_request_endpoints.py

seanpreston · 2022-05-11T18:43:16Z

Hey @PSalant726 — thanks for being thorough, I worry that this PR is a little bloated as you seem to have changed a lot more than is required just to publish the project to PyPi?

PSalant726 · 2022-05-11T19:03:16Z

@ThomasLaPiana

I highly highly strongly recommend adopting the fidesctl docker setup so that in the production image the package is installed as if it was coming directly from pypi, it will catch so many potential packaging errors (such as no init files)

in fidesctl we use multi-stage builds for our dockerfile. This means that when we run our tests we use a production image with fidesctl installed from a clean build, instead of symlinked. This makes sure that our tests run against a package that has been installed as if it were a pip install fidesctl.

I can create another issue to address this.

@seanpreston

I worry that this PR is a little bloated as you seem to have changed a lot more than is required just to publish the project to PyPi?

the linter error fixes were required to get the tests passing once the new __init__.py files existed. The __init__.py files are necessary to publish the complete package to PyPI. If you prefer, I can:

Remove the new __init__.py files and associated fixes from this PR
Create a new PR that adds the __init__.py files and fixes the linter errors
Rebase this PR off of the new PR, such that this PR only 1) adds versioneer and 2) publishes to PyPI

Let me know if you prefer I do that.

seanpreston · 2022-05-16T18:30:43Z

src/fidesops/service/pagination/pagination_strategy_factory.py

@@ -39,7 +46,7 @@ def get_strategy(
    Returns the strategy given the name and configuration.
    Raises NoSuchStrategyException if the strategy does not exist
    """
-    if strategy_name not in SupportedPaginationStrategies.__members__:
+    if not SupportedPaginationStrategies.__contains__(strategy_name):


What's the rationale for moving away from x in y?

The previous pattern was throwing a linter error because .__members__ is not a literal property of Enums (they don't implicitly inherit from dict, nor should they). Inclusion checks in Enums need to be implemented as custom functions.

tests/api/v1/endpoints/test_privacy_request_endpoints.py

seanpreston · 2022-05-16T18:35:18Z

versioneer.py

+        errors = do_setup()
+        errors += scan_setup_py()
+        if errors:
+            sys.exit(1)


Does Versioneer need to be included like this vs. being a pip import itself?

versioneer was added as an import:

fidesops/requirements.txt

Line 36 in f9cf2f0

versioneer==0.19

This script is the result of installing it with versioneer install. See https://github.com/python-versioneer/python-versioneer#theory-of-operation for more info.

seanpreston

Thanks @PSalant726

* Add missing `__init__.py` files * Publish to PyPI on new tag creation * Alphabetize `requirements.txt` There are no version number or constraint changes - only ordering. * Add `versioneer` To automate version numbers * Fix a metric ton of linter errors These errors are being surfaced due to the new `__init__.py` files. * Publish to PyPI on published releases * Test that the `request_id` param actually filters * Update references to the `id` query param to `request_id` * Test publishes to PyPI * Publish to testpypi prior to production

PSalant726 added 3 commits May 10, 2022 16:07

Add missing __init__.py files

0bc39b1

Publish to PyPI on new tag creation

ee17103

Alphabetize requirements.txt

ecfb63d

There are no version number or constraint changes - only ordering.

PSalant726 requested review from seanpreston and ThomasLaPiana May 10, 2022 22:46

PSalant726 self-assigned this May 10, 2022

PSalant726 commented May 10, 2022

View reviewed changes

PSalant726 removed the run unsafe ci checks Triggers running of unsafe CI checks label May 10, 2022

PSalant726 force-pushed the publish-to-pypi branch 2 times, most recently from 8e1740e to 24264d4 Compare May 11, 2022 00:12

PSalant726 marked this pull request as draft May 11, 2022 00:57

PSalant726 force-pushed the publish-to-pypi branch from 24264d4 to 22a31f3 Compare May 11, 2022 02:45

PSalant726 added 2 commits May 10, 2022 20:56

Add versioneer

8df0957

To automate version numbers

Fix a metric ton of linter errors

84bd6b3

These errors are being surfaced due to the new `__init__.py` files.

PSalant726 force-pushed the publish-to-pypi branch from 22a31f3 to 84bd6b3 Compare May 11, 2022 02:58

PSalant726 marked this pull request as ready for review May 11, 2022 03:15

PSalant726 added the run unsafe ci checks Triggers running of unsafe CI checks label May 11, 2022

Publish to PyPI on published releases

1205daa

seanpreston reviewed May 11, 2022

View reviewed changes

pyproject.toml Show resolved Hide resolved

seanpreston reviewed May 11, 2022

View reviewed changes

src/fidesops/api/v1/endpoints/privacy_request_endpoints.py Show resolved Hide resolved

PSalant726 added 3 commits May 11, 2022 16:42

Test that the request_id param actually filters

a98e914

Update references to the id query param to request_id

7343344

Merge branch 'main' into publish-to-pypi

f9cf2f0

PSalant726 requested review from seanpreston and removed request for ThomasLaPiana May 12, 2022 17:22

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels May 16, 2022

seanpreston reviewed May 16, 2022

View reviewed changes

tests/api/v1/endpoints/test_privacy_request_endpoints.py Show resolved Hide resolved

seanpreston reviewed May 16, 2022

View reviewed changes

PSalant726 added 2 commits May 16, 2022 12:54

Test publishes to PyPI

7c74ce7

Publish to testpypi prior to production

e7b4ff2

PSalant726 requested a review from seanpreston May 16, 2022 20:56

seanpreston approved these changes May 16, 2022

View reviewed changes

seanpreston merged commit 6bf5af0 into main May 16, 2022

seanpreston deleted the publish-to-pypi branch May 16, 2022 21:09

PSalant726 mentioned this pull request May 19, 2022

353- initial infra for fideslog integration #541

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publish fidesops to PyPI #491

Publish fidesops to PyPI #491

PSalant726 commented May 10, 2022 •

edited

Loading

PSalant726 left a comment

PSalant726 May 10, 2022

ThomasLaPiana May 11, 2022

PSalant726 May 11, 2022

ThomasLaPiana commented May 11, 2022

ThomasLaPiana commented May 11, 2022

seanpreston commented May 11, 2022

PSalant726 commented May 11, 2022

seanpreston May 16, 2022

PSalant726 May 16, 2022

seanpreston May 16, 2022

PSalant726 May 16, 2022

seanpreston left a comment

Publish fidesops to PyPI #491

Publish fidesops to PyPI #491

Conversation

PSalant726 commented May 10, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

PSalant726 left a comment

Choose a reason for hiding this comment

PSalant726 May 10, 2022

Choose a reason for hiding this comment

ThomasLaPiana May 11, 2022

Choose a reason for hiding this comment

PSalant726 May 11, 2022

Choose a reason for hiding this comment

ThomasLaPiana commented May 11, 2022

ThomasLaPiana commented May 11, 2022

seanpreston commented May 11, 2022

PSalant726 commented May 11, 2022

seanpreston May 16, 2022

Choose a reason for hiding this comment

PSalant726 May 16, 2022

Choose a reason for hiding this comment

seanpreston May 16, 2022

Choose a reason for hiding this comment

PSalant726 May 16, 2022

Choose a reason for hiding this comment

seanpreston left a comment

Choose a reason for hiding this comment

PSalant726 commented May 10, 2022 •

edited

Loading