Refactor simple_deploy.py

[ehmatthes]

The Fly refactoring work in #269 is going well. It's pointing to some simple_deploy refactoring that I want to keep separate from that issue, because it will affect all platforms.

• [x] Work through this entire file, and make sure it reflects clear current thinking about the project.
• [ ] Refactor rest of cli.py.
• [x] Consider moving some methods out of this class to a utils file of some sort.
  • [ ] Consider an sd_utils and a platform_utils.
• [x] Write unit tests for any critical units that are likely to be stable.
• [ ] Write a clear module-level docstring. Discuss public methods, and how this module interacts with things like platform-specific Deployer classes. (Don't check this task until all refactoring work is done; make sure reflects final state of refactoring work.) (Focus on this to after implementing plugin model.)
• [x] Methods like execute_subp_run() should log the commands that are run. That will remove a bunch of extraneous log lines from the platform-specific deploy modules.
• [x] Run black against this file, and any related utils files.
• [ ] Consider writing an ABC for the platform deployer classes. (That's not this file, but it will enforce the expectations that this file has for platform-specific code.) (Save for plugin work.)
• [ ] Check for any necessary documentation updates. (Do this thoroughly in a separate pre-1.0 issue.)
• [x] Merge; probably no new release required.

[ehmatthes]

Documentation update

• [ ] Describe, very broadly, how this class works. What gets called, what does it do, and most importantly, how doe it interact with a PlatformDeployer class?
• [x] Consider moving some methods out of this class to a utils file of some sort.

[ehmatthes]

Testing update

I've been avoiding starting unit tests because I called integration tests unit tests already, and I called e2e tests integration tests. I don't need to wait for renaming those to start actual unit tests.

• [x] Make a directory called real_unit_tests. When renaming other tests, I can easily rename this directory.
• [x] Write actual unit tests for utilities that are pulled out of the Command class.

[ehmatthes]

Working notes

• [x] Update module-level docstring. (first pass)
• [x] Remove unused imports.
• [x] __init__()
• [x] create_parser()
• [x] add_arguments()
• [x] Move public methods to top of file.
• [x] Look at indentation of an entire log file. Any obvious changes to make?
  • [x] Do a fly integration test, and look at resulting log file.
  • [x] Add a test for log info to integration test; only check critical info like DATABASE_URL not being in log.

[ehmatthes]

All functions

• [x] __init__()
• [x] create_parser()
• [x] add_arguments()
• [x] handle()
• [x] _parse_cli_options()
• [x] _start_logging()
• [x] _create_log_dir()
• [x] _validate_command()
• [x] _validate_platform_arg() (removed)
• [x] _inspect_system()
• [x] _inspect_project()
• [x] _ignore_sd_logs()
• [x] _strip_secret_key()
• [x] _find_git_dir()
• [x] _check_git_status()
• [x] _diff_output_clean()
• [x] _get_dep_man_approach()
• [x] _check_using_poetry()
• [x] _get_current_requirements()
• [x] _get_req_txt_requirements()
• [x] _get_pipfile_requirements()
• [x] _get_poetry_requirements()
• [x] _add_simple_deploy_req()
• [x] write_output()
• [x] log_info()
• [x] execute_subp_run()
• [x] execute_command()
• [x] _confirm_automate_all()
• [x] get_confirmation()
• [x] add_packages()
• [x] add_package()
• [x] _add_req_txt_pkg()
• [x] _add_poetry_pkg()
• [x] _check_poetry_deploy_group()
• [x] _add_pipenv_pkg()
• [x] _write_pipfile_pkg()
• [x] commit_changes()

[ehmatthes]

Writing output

• [x] Rework write_output() and log_info() to be logically consistent.
• [x] Make sure unit tests pass.
• [x] Move both of these to utils?
  • I don't think so; log_info() needs access to parsed CLI arg --no-logging, which sets self.log_output. So if we move it to utils, it will still need a reference to the sd Command object.
• [x] Make sure unit tests pass.
• [x] Run one integration test.
• [x] Write an actual unit test, or set of tests, for these functions. (Any method that has been moved to utils.)
  • [x] Requires an editable install of simple_deploy: $ pip install -e .
  • [x] get_string_from_output()
• [x] As much as possible, try to put methods in the order that matches execution flow.
• [x] Have execute_subp_run() command log commands before running them, unless skip_logging.
• [x] Make sure execute_command() does this as well.
• [ ] Add a test to current unit test suite (actual integration tests) that verifies --no-logging has intended effect.

[ehmatthes]

_inspect_project()

• [x] self.project_name -> self.local_project_name?
• [x] Remove old comment about getting project name: # Get project name. There are a number of ways to get the project name; for now we'll assume the root url config file has not been moved from the default location.
• [x] execute_subp_run(), execute_command():
  • [x] Use execute_quick_command() and...
  • [x] execute_slow_command(). This matches why each of these is called, and needed. Name for purpose, rather than implementation.
  • [x] Log commands from these by default. (Logging output is handled by caller, because it's usually written to console and logged.)
  • [x] Are there any commands that aren't currently being logged, that require a skip_logging arg?l
  • [x] Search log_info(
• [x] _check_git_status()
  • [x] Can probably do alll this with git status --porcelain, and not use git diff?
  • [x] Examine clean status
  • [x] simple_deploy_logs/ present: ?? simple_deploy_logs/
  • [x] simple_deploy added to INSTALLED_APPS: M blog/settings.py -> need to run git diff to see what the change was.
  • [x] Logs and simple_deploy added to INSTALLED_APPS: M blog/settings.py\n?? simple_deploy_logs/
  • [x] Are any of these workflows tested? If not, try moving some of the output processing to utility functions, which can be easily unit tested.
  • [x] Allow changes to .gitignore.
• [x] Current unit tests should be able to check git inspection work. Make a change, call simple_deploy, assert error output, checkout original state.
  • [x] Clean git status
  • [x] simple_deploy_logs/ present
  • [x] simple_deploy_logs/ added to .gitignore
  • [x] Added simple_deploy to installed_apps
  • [x] unacceptable settings change
  • [ ] newline at end messages?
  • [ ] blank lines?
  • [x] combinations of above
  • [x] Is capfd needed?
  • [x] Do I need to be resetting the test project? When is that called, ie in other test runs?
• [x] Move all of this work to one utility function that gets all the output needed to determine if it's okay to proceed. Maybe it can all be done through git diff?
  • [ ] call git diff --name-only and inspect output?
  • [ ] call `git diff for each file changed?
• [x] Add explicit output stating results of git check.

[ehmatthes]

Unit testing git status checks

(cont. from above)

• [x] Make each pass when run separately.
  • [x] test_clean_git_status()
  • [x] test_unacceptable_settings_change()
  • [x] test_unacceptable_file_changed()
  • [x] test_sdlogs_exists()
  • [x] test_add_sdlogs_gitignore()
  • [x] test_add_sd_installed_apps()
  • [x] test_sdlogs_exists_add_sdlogs_gitignore()
  • [x] test_sdlogs_exists_sd_installed_apps()
  • [x] test_sdlogs_gitignore_sd_installed_apps()
  • [x] test_sdlogs_exists_sdlogs_gitgnore_sd_installed_apps()
• [x] Sort test suite issues.
• [x] Make sure summary output added for git status check.
• [x] Test --ignore-unclean-git
  • [x] settings change
  • [x] other change
• [x] Reduce nesting in check_status_output()? (No, more redirection probably not helpful.)
• [x] Make sure logs are appropriate.
• [x] Update any other test failures.
• [x] Run integration tests.

[ehmatthes]

Test suite issues

All tests pass in isolation, but suite fails. ie, test_sdlogs_exists() fails. Jumping to the temp dir shows this:

% cd ../../pytest-current/blog_project0    
eric@Erics-Mac-Studio blog_project0 % git status
On branch main
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   .gitignore
    modified:   blog/settings.py
    modified:   blog/wsgi.py
    modified:   requirements.txt

Untracked files:
  (use "git add <file>..." to include in what will be committed)
    .dockerignore
    Dockerfile
    fly.toml

no changes added to commit (use "git add" and/or "git commit -a")

That's a bunch of cruft from previous tests that passed:

platform_agnostic_tests/test_git_status_checks.py::test_clean_git_status[req_txt] PASSED                    [  3%]
platform_agnostic_tests/test_git_status_checks.py::test_unacceptable_settings_change[req_txt] PASSED        [  6%]
platform_agnostic_tests/test_git_status_checks.py::test_unacceptable_file_changed[req_txt] PASSED           [ 10%]
platform_agnostic_tests/test_git_status_checks.py::test_sdlogs_exists[req_txt] FAILED                       [ 13%]

• [x] Make sure temp project is reset after each test run. Look at how this is done in other platform-agnostic tests.
• [x] msp.reset_test_project() has module scope, but should have function scope?
• [x] Okay, scope fun: Can't make reset_test_project() function-scoped without making run_simple_deploy() function scoped as well. So:
  • [x] Make both function-based. Does this make all tests pass? (No, does not seem to be resetting project. :/)
  • [x] Try making both function-scoped. How much does the test suite slow down? (Too much! pytest platforms/ 10s -> 30s)
  • [ ] Try making a function and module scoped version of reset_test_project(), and use each as needed.
• [x] Individual tests do not all pass with function-function scope

[ehmatthes]

More test suite notes

• [x] Move reset_test_project_function() to test_git_status_checks.py module, and set autouse=True. (Good, it's being called every time, but still not letting all tests pass.)
• [x] Find a way to keep run_simple_deploy() from running for this module? Maybe a conditional autouse? Even though it's not doing anything, it's causing tests to run 3x! (Override fixture in this specific module; SO answer.)
• [x] Pass test_unacceptable_settings_change individually.
• [x] Pass test_unacceptable_settings_change as part of suite.

[ehmatthes]

New tests

• [x] Test that simple_deploy_logs/ is added to .gitignore? (Already done.)
• [x] Test utils for parsing requirements:
  • [x] parse_req_txt()
  • [x] pipenv
  • [x] poetry
• [ ] Need to add toml to requirements.txt?

[ehmatthes]

Git status checks

From a unit test run, this output is failing the git status check:

% git status --porcelain
 M .gitignore
(b_env) eric@Erics-MacBook-Air blog_project0 % git diff --unified=0
diff --git a/.gitignore b/.gitignore
index 9c96d1b..4279ffb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -8,0 +9,3 @@ db.sqlite3
+
+# Ignore logs from simple_deploy.
+simple_deploy_logs/

• [x] Reproduce: Run fly_io unit test, then jump into test project and rerun configuration command. Should fail with above as the main issue, not passing the git status check. (No, a passing unit test doesn't lead to this issue. Make manual changes to reproduce this.)
• [x] Sort out why existing (real) unit test is not catching this. (It wasn't ignoring the # Ignore logs... comment.)
• [x] Update test so it fails because of the existing issue.
• [x] Update code so it passes the test. (Removed that comment from .gitignore.)

[ehmatthes]

Move call to prep_automate_all()

Right now, prep_automate_all() is called from simple_deploy. The only thing after it is adding simple_deploy as a requirement. This is a bunch of complexity with little benefit. The only benefit was not adding simple_deploy as a requirement until after prepping automate-all. But we've already added a log file. People can use Git to undo changes if they decide to abandon simple_deploy.

• [x] Run integration tests.
• [x] Move call to prep_automate_all() from simple_deploy's handle() to PlatformDeployer.deploy().
• [x] Call validate_platform() from deploy() as well.
• [x] Run all local tests. (One Heroku test fails, but I'll deal with that when refactoring Heroku.)
• [x] Run integration tests.
  • [x] fly_io
  • [x] platform_sh
  • [x] heroku

[ehmatthes]

Final handle() cleanup

• [x] Can platform_deployer() be created later, now that most platform-specific work has been moved out of simple_deploy.py? (Yes. It means making a couple minor modifications to user's project before confirming preliminary status, but these can easily be rolled back by user.)
• [ ] Note: further refactoring could break this up into a class that handles package management issues. But the plugin model is higher priority. (Agreed, not now.) (Noted in #14)
• [x] Does add_package() already check if the package is present? If so, remove that check from each of the pkg_manager-specific methods.
• [x] Move req_txt work to utils, like other pkg_manager approaches.
• [x] Write unit test for req_txt util.
• [x] Write unit test for pipfile util.
• [x] Remove comments "Added by simple_deploy." That's in the git history, and just clutters a requirements file.
• [x] Run an integration test with --automate-all.

[ehmatthes]

_add_poetry_pkg()

• [x] Looks like check for poetry deploy group will fail if there's any slight formatting differences.
  • [x] Write test for this?
  • [x] Update code to more reliably check for a pre-existing deploy group? (Use toml library.)
• [x] Run Heroku poetry integration test.
• [x] If it passes, update unit test to pass.
• [x] Finish refactoring poetry methods, including moving some of the work to utils.
• [x] Write real unit tests for new util functions.

[ehmatthes]

Misc cleanup

• [x] Convert some util functions to helpers.
• [x] Finish testing git status processing, including refining behavior of checks.

[ehmatthes]

Parking lot: git status checks

I saw one issue around newlines, with a diff that included a + and - of the same line. I'm not sure if that would be a common thing, or if that was a weird one-off issue. Keep this out for now, but keep it here in case it comes up again.

def test_clean_diff():
    diff_output = dedent(
        """\
        diff --git a/blog/settings.py b/blog/settings.py
        index 6d40136..6395c5a 100644
        --- a/blog/settings.py
        +++ b/blog/settings.py
        @@ -39,0 +40 @@ INSTALLED_APPS = [
        +    'simple_deploy',
        @@ -134 +135 @@ DEFAULT_AUTO_FIELD = 'django.db.models.BigAutoField'
        -LOGIN_URL = 'users:login'
        \\ No newline at end of file
        +LOGIN_URL = 'users:login'"""
    )

    cleaned_diff = sd_utils._clean_diff(diff_output.splitlines())
    assert cleaned_diff == ["+    'simple_deploy',"]

def _clean_diff(diff_lines):
    ...
    # Ignore changes that relate to newlines, like this:
    #     -LOGIN_URL = 'users:login'            <-- this line
    #     \\ No newline at end of file
    #     +LOGIN_URL = 'users:login'"""         <--- and this line
    # Remove the + or -, and find any lines that are repeated.
    changes = [l[1:] for l in lines]
    changes_to_remove = [c for c in changes if changes.count(c) == 2]
    lines = [l for l in lines if l[1:] not in changes_to_remove]

[ehmatthes]

Move remaining platform-specific work to platform-specific module

The only platform-specific work that's still being done in simple_deploy is confirming the user wants to use a platform that only has preliminary support. But that work can be done after platform-agnostic work is done. This means all platform-specific work is done entirely in the platform-specific deployer module.

• [x] Move instantiation of platofrm_deployer object to end of handle().
• [x] Remove all references to platform-specific messages?
• [x] Move confirm_automate_all() to platform-specific modules.
• [x] Call confirm_preliminary() from platform-specific deployer modules.
  • [x] Display preliminary message even when unit testing.
• [x] Run unit tests.
• [x] Run integration tests.
  • [x] fly_io
  • [x] fly_io automate_all
  • [x] platform_sh
  • [x] heroku
• [x] Do a manual deploy to fly using automate_all.
• [ ] --automate-all needs some unit tests! (Noted in #285)
• [ ] Some of _confirm_automate_all() should be moved to either simple_deploy, or maybe even utils.
  • [ ] Or a parent class? (Noted in #14)

[ehmatthes]

fly automate_all test

• [x] Sort out _confirm_use_attached_db() arg issue. Should it return True during automate-all runs?

[ehmatthes]

Manual run: fly automate_all

• [x] Confirmed fly, but didn't confirm automate all until after creating app and db! Fix this.