Refactor Heroku deploy.py

ehmatthes commented 10 months ago

Once working, there's some refactoring to do.

First tasks

[x] Run black to start.
[x] Use --json wherever possible when parsing CLI calls. Start with apps:info.
[x] Refactor deploy.py, based on experience of refactoring fly and simple_deploy.py.
[x] Use a more consistent style for docstrings. See PEP 257. See also _get_deployed_project_name() as an example.
[x] Run black.
[ ] Run tests.
- [x] local
- [ ] e2e
[ ] Quick look at documentation
[ ] Update CLI messages.
[ ] Merge.

Second tasks

This is the last platform to be refactored. Go back through all three platforms, looking for utilities that can be pulled out into sd_utils. Any utility functions that can be put there can be much more reliably implemented and tested than leaving it to each platform to handle. This also creates much more consistent workflows across the different platforms that will be supported.

[ ] Move remaining work to a new issue, and close this one.
[ ] Use Parking Lot to close 5-10 oldest issues.
[ ] Look at what's done across the different platform deploy scripts. Identify any utility functions that can be moved to sd_utils. Move them there, and make sure all work in the three deployer scripts is unique to that platform.
[ ] Consider writing quick unit tests for each new utility function.

ehmatthes commented 4 months ago

Methods

[x] __init__()
[x] deploy()
[x] _confirm_automate_all() (Moved to simple_deploy.py)
[x] _validate_platform()
- [x] Need to check that a project has been created, or using automate_all here, not later.
[x] handle_poetry() Good enough for now. This round of refactoring is focused on "Can someone else make sense of current approach, and build on what's been started?"
[x] _prep_automate_all()
[x] _get_heroku_app_info()
[x] _set_heroku_env_var()
[x] _get_heroku_settings()
[x] _check_allowed_hosts()
[x] _add_db_settings()
[x] _add_static_file_settings()
[x] _check_current_heroku_settings()
[x] _add_heroku_setting()
[x] _prep_heroku_setting()
[x] _add_gunicorn()
[x] _configure_db()
[x] _add_db_packages()
[x] _configure_static_files()
[x] _configure_debug()
[x] _configure_secret_key()
[x] _generate_procfile()
[x] _add_static_file_directory()
[x] _conclude_automate_all()
[x] _summarize_deployment()
[x] _show_success_message()
[x] _generate_summary()

ehmatthes commented 4 months ago

More tasks

[x] Move confirm_automate_all() to simple_deploy.py? It doesn't seem to have any platform-specific behavior at this point, and it seems like it could be called right before calling the platform-specific deployer. (Don't do this, the platform does have a specific confirmation message. The method could be moved to simple_deploy, but the message would still have to come from the platform-specific code. That's probably a good thing to do. Maybe specify the plugin has to provide a confirmation message for automate_all.
- [x] Start drafting the contract between the host and the plugins. See contributing/architecture_notes.md.
- [x] Moved confirmation to simple_deploy, which has access to the appropriate confirmation message through the platform-specific deployer object.
- [x] Merged changes to this point, to not let this branch drift too far from main.
[ ] Address the six heroku tests that have been failing for a while now.
[ ] Improve consistency across platforms.
- [ ] Specify a preferred order for tasks, in the draft plugin contract.
- [ ] Ensure that all three deployers follow that order.
- [ ] This should be a matter of looking at the deploy() method.

ehmatthes commented 4 months ago

`_validate_platform()`

[x] This method needs to verify a project was created, or we're using automate_all.
[x] Verifying a project was created should be moved out of _get_heroku_app_info().
[x] Also, check that CLI is installed, and user is authenticated.
[x] Make sure automate-all path does not conflict with logic in get_heroku_app_info().
- [x] That method may need to be renamed more specifically to a db-focused name.
[x] Postgres mini is being deprecated. Using essential-0 now. See Choosing the right heroku postgres plan, Heroku postgres pricing. ~$5/month.

ehmatthes commented 4 months ago

Ongoing tasks

[x] When unit testing, self.heroku_app_name is not set.
[ ] Note archtiecture recommendations somewhere appropriate (not a new issue).

ehmatthes commented 4 months ago

Simplify approach to settings

This is a much simpler approach to idempotency, and multiple runs. People will do multiple runs if something fails and they have to modify their project. Avoid that if at all possible, but keep our workflow as simple as possible.

[x] Rather than dealing with individual settings, just build a platform-specific settings block and add it at the end of the file.
- [x] Get current settings.
- [x] Build a platform-specific settings block.
- [x] Add all necessary settings to the block.
- [x] Add entire block to settings.
- [x] Rewrite settings file.
- [x] Make sure local tests pass.
- [x] Remove _add_heroku_setting().
- [x] Even simpler approach: Add a single settings block from a template, just like Platform.sh does.
- [x] Clarify where exactly to check for existing settings, and whether to do that during local testing.
- [x] Use an re to remove existing settings block.
[x] If we find a block with the same start line, ask if it's okay to overwrite that block. This should happen before making other changes.
- [x] Use takewhile() to keep only non-platform settings.
- [x] Place this check in validate_platform(), after CLI checks, before checking for a project we can push to.
[x] See if sd_utils can offer a helper function for managing this for multiple platforms.
[ ] Clarify suggestion for an unclean Git status.
[ ] Test.
[ ] Consistency:
- [ ] Use modify_settings().
- [ ] Use same approach of asking to overwrite existing platform-specific settings block, if found. Can this be moved to sd_utils, or simple_deploy?
[ ] Implement for Fly.
[ ] Implement for Platform.sh.
[ ] Merge after doing this, to not diverge far from main when affecting other platforms.

ehmatthes commented 4 months ago

Simplify approach to requirements

[x] Use add_packages()?
[x] Similar to settings, don't add requirements one at a time. Add them all as a block.
~~[ ] Let simple_deploy pass responsibility for adding django-simple-deploy requirement to platform deployer? It would be nice if all simple_deploy does is add log, and modify .gitignore.~~ Don't do this. A platform should only be responsible for its own needs.
[ ] Implement for Fly.
[ ] Implement for Platform.sh.
[ ] Merge.

ehmatthes commented 4 months ago

Simplify overall approach

Originally, I got caught up in replicating the configuration workflow that people do when following a platform's docs. There's no need to do that, and it's much more verbose than it needs to be.

The process really boils down to, and should look as simple as:

[x] Validate that the user's environment is set up to push to this platform, ie that the CLI is installed and authenticated, and any prerequisite resources have been created. This can include an app to deploy to, an env var for an API key, and other things that should be in place before any configuration changes are made.
[x] Add platform-specific requirements.
[x] Set env vars on the platform.
[x] Modify settings.
[x] Make any other platform-specific configuration changes.

ehmatthes commented 4 months ago

Misc

[ ] Looks like Platform.sh is confirming automate all twice, once in sd and once in its own script?

ehmatthes commented 4 months ago

Run e2e test

After first full pass refactoring heroku deploy.py, before porting some changes to other platforms.

[x] Run configuration-only e2e test.

Failed:

Migrating deployed project...
Running python manage.py migrate on ⬢ stark-coast-54964... up, run.2010 (Basic)
Traceback (most recent call last):
  File "/app/manage.py", line 22, in <module>
    main()
  File "/app/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/app/.heroku/python/lib/python3.12/site-packages/django/core/management/__init__.py", line 446, in execute_from_command_line
    utility.execute()
 ...
  File "/app/.heroku/python/lib/python3.12/site-packages/django/db/backends/base/base.py", line 262, in connect
    conn_params = self.get_connection_params()
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/.heroku/python/lib/python3.12/site-packages/django/db/backends/postgresql/base.py", line 174, in get_connection_params
    raise ImproperlyConfigured(
django.core.exceptions.ImproperlyConfigured: settings.DATABASES is improperly configured. Please supply the NAME or OPTIONS['service'] value.
  Opening project...

[x] Troubleshoot above issues by referencing updated heroku docs.
- [x] Updated db settings.
- [x] Database was just not ready. Either use --wait flag when generating the add-on; (This worked; takes ~2-3 min for db to be ready.)
- [ ] or try using the release command in Procfile.
  - [x] Run automate-all e2e tests.
  - [x] Run pypi configuration-only e2e test. (Fails, as expected.)
  - [x] Check heroku dashboard for testing artifacts.
  - [x] Fix issue with local functionality test, around logout.

ehmatthes commented 4 months ago

Make a new release

[x] Heroku works again, so go ahead and make a new release.
[x] Test.
[x] Announce.

ehmatthes commented 4 months ago

All open tasks moved to #319.

ehmatthes / django-simple-deploy