Here's what we would do in septentrion migrate, in order.
The idea is that as much as possible, we avoid mixing decision making and I/O.

• Create the migration table if not exsists
• Read existing migrations in the DB
• Read existing migration versions and filenames in the file system
• Decide whether to initialize the DB with a schema
• Compute the list of migrations to run
• migrate(fixtures before schema)
• migrate(schema)
• migrate(fixtures after schema)
• For each migration:
  • Fake it if boolean says to fake it or
  • migrate(migration)
  • Write the migration in the table as executed

def migrate(migration):

• Read the migration file content
• Parse the migration (meta, sqlparse statements, clean (\timing), etc)
• For each statement (including meta loops):
  • Execute it

setup.cfg should be read before configuration files in ~ or /etc/

Using Github Actions will allow us to drop Travis, which has the drawback to be an third-party service, and lacks support for Windows. Besides, as Procrastinate already uses Github actions, we will soon be able to factor the two pipeline (is will soon be implemented in Github Actions).

Even if the app is mainly designed as a CLI, I'm guessing it might benefit from being usable as a lib too, and it's much better if we avoid global states.

The project is Py3.6+ which means we can add type annotations everywhere. Mypy is already installed, so let's do this.

• Make it easy to be called from outside without too many idiosyncrasies
• Give knobs to allow customizing non-critical parts of the workflow (tracking migrations, displaying, etc)

Use importlib_metadata rather than pkg_resources in __init__.py.

This is the way forward

In my opinion, when used as a library, septentrion should not write anything to stdout by default.

The current behavior is to print to stdout. This can be disabled using a quiet parameter.
I would reverse the logic and write to stdout only when a parameter is passed (e.g. enable_stdout ?)

It seems that if we've already deployed migration for version A.B.C and we add a new migration on version A.B.C, it doesn't apply it, since 0.7.0.

This seems linked to https://github.com/peopledoc/septentrion/blob/master/septentrion/core.py#L157 and how since excludes the first version. Not sure why it doesn't work now and it used to work.

• Add license whoops :/
• Updgrade tooling (mypy black flake8 isort, the 4 horsetools of the apocalypse) (and check-manifest as a useful party)
• Adjust CODEOWNERS
• Add documentation
• Write documentation -> #27
• Add type annotations -> #28
• Add release script
• Complete coverage -> #29
• Add docker compose dev env
• Add code of conduct
• Exclude tests from packaging
• Review code (because our practice has evolved) -> #29

If the septentrion port number is an int, subprocess.run which launches psql fails with a traceback.

This can be solved by making sure the port number, as well as all other environment injected values, are strings.

@ThomasEcuer is on it :)

We currently replace all whitespace (multiple spaces, tabs and newlines) with whitespace, here:

I'm not sure there's a reason for doing this, we could stop doing that I think.

Anyone has an opinion ?

I haven't tested it, but I wonder what the output looks like when not connected to a TTY. I believe colorama behavies itself, and doesn't output colors. Though, we also do a fair share of line rewriting and we should maybe avoid this.

Note: I believe sys.stdout.isatty might be helpful, but there's maybe something more advanced in click or colorama to help us.

To Do:

• Add black and isort, add a CI (travis ? circle ?): #6
• Convert csv parameters to list in click (e.g. ADDITIONAL_SCHEMA_FILES): #7
• Fix relative import bug in python 2: #7
• Understand why WEST_PASSWORD doesn't work: #7
• Rework logs/print/outputs and add logging everywhere: #7
• Embed west init into west migrate when applicable: #7
• Make west fake as a separate command: #7
• Unify stdout with click.style everywhere we need it (improved in #7)
• And obviously, write (py-)tests, loads of 'em, unit, functional, acceptance tests, compute our coverage and make it a big number
• Write documentation (see Daniele Procida's talk on how to organize a good technical documentation)
• Contributing doc too
• Publish on PyPI ! before the name is taken by someone else
• Communicate around the project

When referencing a folder in NORTH_BEFORE_SCHEMA_FILES and NORTH_AFTER_SCHEMA_FILES, all files in it should be executed in sorted order

Code linting and doc linting are currently broken, which prevents CI from correctly finishing.

It's fairly easy to notice if there are unapplied migrations that are not at the end of the list. We should ask the user whether they want to run them or not. Also, there should be a CLI parameter indicating if we want to always do, or not do, or ask, so that we can automate this a bit better

With Septentrion 0.1.3 and sqlparse 0.4.1 we get this error at migration time:

Version 20.10
  [ ] Applying 20.10-0-version-ddl.sql ...Traceback (most recent call last):
  File "/home/nova/.virtualenvs/doctype_prediction/bin/septentrion", line 8, in <module>
    sys.exit(main())
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/septentrion/cli.py", line 184, in migrate_func
    migrate.migrate(stylist=style.stylist)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/septentrion/migrate.py", line 56, in migrate
    run_script(path)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/septentrion/migrate.py", line 143, in run_script
    script.run(db.get_connection())
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/septentrion/runner.py", line 118, in run
    block.run(cursor)
  File "/home/nova/.virtualenvs/doctype_prediction/lib/python3.7/site-packages/septentrion/runner.py", line 48, in run
    raise SQLRunnerException("sqlparse failed to properly split input")
septentrion.runner.SQLRunnerException: sqlparse failed to properly split input
script returned exit code 1

There wasn't any problem with sqlparse 0.3.1. So it looks like there's a change in sqlparse that breaks Septentrion.

Because config file only sets defaults values, and target-version is required=True, setting target-version in septentrion.ini results in Error: Missing option "--target-version".

In order to make septentrion CLI interface close to its lib interface, we should remove as much decisions as possible from the cli, making it a simple wrapper.

This involves:

• Checking that versions are in the correct format
• ...?

It should default to the latest version

As a follow-up of #25

The base system is already in place, and if you need inspiration for the style, see https://github.com/peopledoc/procrastinate

This was found by @k4nar.

It seems that when we start with a scheme corresponding to version N, the migrations before N are not run, but they're still displayed, and they should not be.

Let's fix this.

https://click.palletsprojects.com/en/7.x/api/#click.version_option

To reproduce, call septentrion CLI with --config-file option, e.g.:
septentrion --config-file=some_file.ini --target-version=1.0 migrate

It will fail:

  ...
  File "/tmp/venv/lib/python3.7/site-packages/septentrion/cli.py", line 37, in load_config
    ctx.default_map = configuration.load_configuration_files(value)
  File "/tmp/venv/lib/python3.7/site-packages/septentrion/configuration.py", line 193, in load_configuration_files
    return parse_configuration_file(file_contents)
  File "/tmp/venv/lib/python3.7/site-packages/septentrion/configuration.py", line 82, in parse_configuration_file
    parser.read_string(content)
  File "/usr/local/lib/python3.7/configparser.py", line 721, in read_string
    sfile = io.StringIO(string)
TypeError: initial_value must be str or None, not bytes
script returned exit code 1

This deserves a discussion, I'm not sure about it.

We now have Version objects, but they're currently used only when comparing versions, etc. The rest of the stack passes versions as strings.
I guess it would be interesting to use Version objects all the way in the stack.

Pros:

• We currently parse them several times (but this is negligable computation time)
• There would be less ambiguity regarding the type expected for version arguments (but we have type annotations anyway)
• Bugs where we pass a string that is not a proper version would not be possible, thanks to static typing

Cons:

• We'd have to import the Version type everywhere, including in the tests etc.

Note:

• I don't think it's a good idea to expose this to our external API. Strings are better suited than internal types to ease use, but this doesn't apply to internal calls.

Due to recent vulnerability for celery (see) it better to upgrade it to version 5.2.2.
But it's impossible to integrate it with celery 5.2.2 which has click>=8.0.0,<9.0 (requirements.txt)
Septentrion requires click package >=7.1.2,<8.0.0
We should update the requirements or even unpin the versions for click 🤔

There's a warning in the tests:

tests/integration/test_migration.py::test_init_schema_extra_files
  /home/joachim/src/septentrion/septentrion/migration.py:116: UserWarning: ADDITIONAL_SCHEMA_FILES will be deprecated. Use BEFORE_SCHEMA_FILES instead.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/latest/warnings.html

We should catch it and check it, rather than letting it bubble.

Here's the pytest relevant doc: https://docs.pytest.org/en/stable/warnings.html

It's quite nice to work with a 100% test coverage. It's not very hard to achieve on small codebases, and it gives a nice incentive to keep writing tests.

Also, I think a thorough code review would be interesting. I bet there are some things we've learned to do better.

If interpreter goes there: https://github.com/peopledoc/septentrion/blob/master/septentrion/cli.py#L42
On the line https://github.com/peopledoc/septentrion/blob/master/septentrion/cli.py#L49 it raises UnboundLocalError, but not the exceptions.NoSeptentrionSection.

Currently we need schema file to run a migration with septentrion. But when creating a project from scratch, the initial schema is the first migration.

Did we want to force the user to have schema file to initialize the database with Septentrion or do we want to allow initial migrations with only migrations files ?

@dsteinberger found an issue in the lib.py code:

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/threading.py", line 973, in _bootstrap_inner
    self.run()
  File "/usr/local/lib/python3.9/threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "/opt/virtualenv/lib/python3.9/site-packages/django/utils/autoreload.py", line 64, in wrapper
    fn(*args, **kwargs)
  File "/opt/virtualenv/lib/python3.9/site-packages/django/core/management/commands/runserver.py", line 121, in inner_run
    self.check_migrations()
  File "/opt/virtualenv/lib/python3.9/site-packages/django_north/management/commands/runserver.py", line 18, in check_migrations
    migration_plan = septentrion.build_migration_plan(
  File "/opt/virtualenv/lib/python3.9/site-packages/septentrion/lib.py", line 35, in build_migration_plan
    return core.build_migration_plan(
TypeError: build_migration_plan() got an unexpected keyword argument 'schema_version'