Check for uncommitted files before running sync (#869)

* Update core.py

* move gitpython from a dev requirement to a full requirement

* re-add GitPython as a dev-requirement since it is required by nox for builds

* remove accidental change

* only throw an error for unstaged commits, not all uncommitted

* add a more helpful error message

* fix the dirty phrase to detect properly

* inject the analytics_id env var into the container, don't write the analytics_id to the toml file if it is set as an env var

* clarify naming for opt_out vars

* add tests for the new git_is_dirty function

* update the git_is_dirty to also catch new (untracked) files

* update the changelog, fix the cli sync test

* update sync docstring

CHANGELOG.md

@@ -28,6 +28,7 @@ The types of changes are:
   * System scanning results: AWS systems are stored and can be selected for review
 * Added Cypress for testing [713](https://github.com/ethyca/fides/pull/833)
 * CustomInput type "password" with show/hide icon.
+* Sync CLI command now checks for untracked/unstaged files in the manifests dir [#869](https://github.com/ethyca/fides/pull/869)
 * Add Okta support to the `/generate` endpoint [#842](https://github.com/ethyca/fides/pull/842)
 * Add db support to `/generate` endpoint [849](https://github.com/ethyca/fides/pull/849)
 * Added OpenAPI TypeScript client generation for the UI app. See the [README](/clients/admin-ui/src/types/api/README.md) for more details.
@@ -40,6 +41,7 @@ The types of changes are:
 * Initial configuration wizard UI view
   * Refactored step & form results management to use Redux Toolkit slice.
 * Remove the `obscure` requirement from the `generate` endpoint [#819](https://github.com/ethyca/fides/pull/819)
+* Remove the `obscure` requirement from the `generate` endpoint [#819](https://github.com/ethyca/fides/pull/819)
 
 ### Docs
 

docker-compose.yml

@@ -29,6 +29,7 @@ services:
       FIDESCTL_TEST_MODE: "True"
       FIDESCTL__CLI__SERVER_HOST: "fidesctl"
       FIDESCTL__CLI__SERVER_PORT: "8080"
+      FIDESCTL__CLI__ANALYTICS_ID: ${FIDESCTL__CLI__ANALYTICS_ID}
       FIDESCTL__API__DATABASE_HOST: "fidesctl-db"
 
   fidesctl-ui:

requirements.txt

@@ -8,6 +8,7 @@ fastapi==0.77.1
 fideslang==0.9.0
 fideslib==2.2.2
 fideslog==1.1.5
+GitPython==3.1
 loguru>=0.5,<0.6
 openpyxl==3.0.9
 pandas==1.4

src/fidesctl/cli/commands/core.py

@@ -7,12 +7,13 @@
     manifests_dir_argument,
     verbose_flag,
 )
-from fidesctl.cli.utils import pretty_echo, print_divider, with_analytics
+from fidesctl.cli.utils import echo_red, pretty_echo, print_divider, with_analytics
 from fidesctl.core import apply as _apply
 from fidesctl.core import audit as _audit
 from fidesctl.core import evaluate as _evaluate
 from fidesctl.core import parse as _parse
 from fidesctl.core import sync as _sync
+from fidesctl.core.utils import git_is_dirty
 
 
 @click.command()
@@ -141,11 +142,18 @@ def parse(ctx: click.Context, manifests_dir: str, verbose: bool = False) -> None
 def sync(ctx: click.Context, manifests_dir: str) -> None:
     """
     Update local resource files by their fides_key to match their server versions.
+
+    Aborts the sync if there are unstaged or untracked files in the manifests dir.
     """
 
     config = ctx.obj["CONFIG"]
     # Do this to validate the manifests since they won't get parsed during the sync process
     _parse.parse(manifests_dir)
+    if git_is_dirty(manifests_dir):
+        echo_red(
+            f"There are unstaged changes in your manifest directory: '{manifests_dir}' \nAborting sync!"
+        )
+        raise SystemExit(1)
     _sync.sync(
         url=config.cli.server_url,
         manifests_dir=manifests_dir,

src/fidesctl/cli/utils.py

@@ -101,11 +101,18 @@ def check_and_update_analytics_config(ctx: click.Context, config_path: str) -> N
             user={"analytics_opt_out": ctx.obj["CONFIG"].user.analytics_opt_out}
         )
 
-    if ctx.obj["CONFIG"].user.analytics_opt_out is False and get_config_from_file(
+    is_analytics_opt_out = ctx.obj["CONFIG"].user.analytics_opt_out
+    is_analytics_opt_out_config_empty = get_config_from_file(
         config_path,
         "cli",
         "analytics_id",
-    ) in ("", None):
+    ) in ("", None)
+    is_analytics_opt_out_env_var_set = getenv("FIDESCTL__CLI__ANALYTICS_ID")
+    if (
+        not is_analytics_opt_out
+        and is_analytics_opt_out_config_empty
+        and not is_analytics_opt_out_env_var_set
+    ):
         config_updates.update(cli={"analytics_id": ctx.obj["CONFIG"].cli.analytics_id})
 
     if len(config_updates) > 0:

src/fidesctl/core/utils.py

@@ -12,6 +12,7 @@
 import sqlalchemy
 from fideslang.models import DatasetField, FidesModel
 from fideslang.validation import FidesValidationError
+from git.repo import Repo
 from sqlalchemy.engine import Engine
 
 logger = logging.getLogger("server_api")
@@ -117,3 +118,17 @@ def sanitize_fides_key(proposed_fides_key: str) -> str:
     """
     sanitized_fides_key = re.sub(r"[^a-zA-Z0-9_.-]", "_", proposed_fides_key)
     return sanitized_fides_key
+
+
+def git_is_dirty(dir_to_check: str = ".") -> bool:
+    """
+    Checks to see if the local repo has unstaged changes.
+    Can also specify a directory to check.
+    """
+    repo = Repo()
+    git_session = repo.git()
+
+    dirty_phrases = ["Changes not staged for commit:", "Untracked files:"]
+    git_status = git_session.status(dir_to_check).split("\n")
+    is_dirty = any(phrase in git_status for phrase in dirty_phrases)
+    return is_dirty

tests/cli/test_cli.py

@@ -1,11 +1,11 @@
 # pylint: disable=missing-docstring, redefined-outer-name
 import os
 from pathlib import PosixPath
-from shutil import copytree
 from typing import Generator
 
 import pytest
 from click.testing import CliRunner
+from git.repo import Repo
 from py._path.local import LocalPath
 
 from fidesctl.cli import cli
@@ -102,13 +102,19 @@ def test_dry_diff_apply(test_config_path: str, test_cli_runner: CliRunner) -> No
 def test_sync(
     test_config_path: str, test_cli_runner: CliRunner, tmp_path: PosixPath
 ) -> None:
-    copytree("demo_resources", tmp_path, dirs_exist_ok=True)
-    result = test_cli_runner.invoke(
-        cli, ["-f", test_config_path, "sync", str(tmp_path)]
-    )
+    """
+    Due to the fact that this command checks the real git status, a pytest
+    tmp_dir can't be used. Consequently a real directory must be tested against
+    and then reset.
+    """
+    test_dir = "demo_resources/"
+    result = test_cli_runner.invoke(cli, ["-f", test_config_path, "sync", test_dir])
     print(result.output)
     assert result.exit_code == 0
 
+    git_session = Repo().git()
+    git_session.checkout("HEAD", test_dir)
+
 
 @pytest.mark.integration
 def test_audit(test_config_path: str, test_cli_runner: CliRunner) -> None:

tests/core/test_utils.py

@@ -1,4 +1,5 @@
 # pylint: disable=missing-docstring, redefined-outer-name
+import os
 from pathlib import PosixPath
 from typing import Generator
 
@@ -90,3 +91,25 @@ def test_sanitize_fides_key(fides_key: str, sanitized_fides_key: str) -> None:
 )
 def test_check_fides_key(fides_key: str, sanitized_fides_key: str) -> None:
     assert sanitized_fides_key == utils.check_fides_key(fides_key)
+
+
+@pytest.mark.unit
+class TestGitIsDirty:
+    """
+    These tests can't use the standard pytest tmpdir
+    because the files need to be within the git repo
+    to be properly tested.
+
+    They will therefore also break if the real dir
+    used for testing is deleted.
+    """
+
+    def test_not_dirty(self) -> None:
+        assert not utils.git_is_dirty("tests/data/example_sql/")
+
+    def test_new_file_is_dirty(self) -> None:
+        test_file = "tests/data/example_sql/new_file.txt"
+        with open(test_file, "w") as file:
+            file.write("test file")
+        assert utils.git_is_dirty()
+        os.remove(test_file)