Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cli] new feature: 'migrate info': warn about changed-after-installed migrations #870

Closed
salewski opened this issue Dec 3, 2020 · 8 comments · Fixed by #1680
Closed
Labels
cli Concerns `sqlx-cli` E-easy enhancement New feature or request migrations Proposals or bugs involving migrations

Comments

@salewski
Copy link
Contributor

salewski commented Dec 3, 2020

    $ sqlx --version
    sqlx-cli 0.2.0

    $ git rev-parse --short=12 HEAD
    7f1bff406d94

Related to:

Related tangentially to:


It would be useful to have sqlx migrate info warn the user about migrations whose file checksum no longer matches the value recorded in the db, similar to how sqlx database setup does.

For example, if I:

  1. sqlx database setup
  2. change the migration file
  3. sqlx database setup

Upon the second setup invocation an error is emitted:

    error: migration 20201202222356 was previously applied but has been modified

That's quite nice.

From the perspective of treating the sqlx migrate info command as the thing to use to answer the question, "Where does the database stand with respect to the available migrations?", it would be nice if it were able to provide similar information. Currently it shows that the migration was successfully installed at some point in the past, but does not give any indication that the migration script has been modified:

    $ sqlx migrate info
    20201202222356/installed name-of-migration
@mehcode mehcode added cli Concerns `sqlx-cli` E-easy enhancement New feature or request migrations Proposals or bugs involving migrations labels Dec 3, 2020
@mehcode
Copy link
Member

mehcode commented Dec 3, 2020

This is an excellent idea.

Here is how Flyway does info for an example of more info we could provide too.

+------------+---------+---------------------+------+---------------------+---------+----------+
| Category   | Version | Description         | Type | Installed On        | State   | Undoable |
+------------+---------+---------------------+------+---------------------+---------+----------+
| Versioned  | 1       | Create person table | SQL  | 2017-12-21 18:05:10 | Success | No       |
| Versioned  | 2       | Add people          | SQL  | 2017-12-21 18:05:10 | Success | No       |
| Repeatable |         | People view         | SQL  | 2017-12-21 18:08:29 | Success |          |
+------------+---------+---------------------+------+---------------------+---------+----------+

I think I would suggest changing the state of the migration and indicating its a failure state with a red color:

    $ sqlx migrate info
    20201202222356/dirty name-of-migration

In the same vein, I'd also suggest adding the name-of-migration to that particular error message:

error: migration 20201202222356 "name-of-migration" was previously applied but has been modified

@abonander
Copy link
Collaborator

abonander commented Dec 3, 2020

This could also be useful as sqlx migrate check which returns a nonzero exit code if any migrations failed or have not been applied. Maybe a positive number if there are just unapplied migrations, or a negative number if there are some failed migrations.

Edit: that may be an antipattern though as exit codes on Unix are unsigned mod 256 and some are already reserved for some common uses: https://tldp.org/LDP/abs/html/exitcodes.html

@salewski
Copy link
Contributor Author

salewski commented Dec 3, 2020

This could also be useful as sqlx migrate check which returns a nonzero exit code if any migrations failed or have not been applied.

I do like the idea of conveying specific statuses (more than just success/non-success) through the exit status where it makes sense to do so, but it is probably a good idea to keep the total universe of such codes small -- probably less than five, and almost certainly less than 10, but who knows...

For a general purpose "check" command, it would probably be desirable at some point to convey richer information than can be conveyed through exit status numbers. For those use cases, it is likely to be better to have a small number of exit status codes to indicate coarse status ("ran successfully", "couldn't connect", "no local migrations present", etc.), and emit the richer detail (perhaps in a machine readable format) on stdout (or something).

For comparison with some widely used tools:

  • Debian's lintian uses three exit status codes: 0, 1, and 2. From lintian(1):

    EXIT STATUS
           0   Normal operation.
    
           1   Lintian run-time error. An error message is sent to stderr.
    
           2   Detected a condition specified via the --fail-on option. This can
               be used to trigger a non-zero exit value in case of policy
               violations.
    

    By default the tool exits with status 2 for checks with a default level of error, but that --fail-on option allows the user to request that it exit with status 2 for warning, info, etc. As a general check tool, I think this is a pretty good example to emulate.

  • GNU grep also uses the three status codes: 0, 1, and 2. From grep(1):

    EXIT STATUS
           Normally the exit status is 0 if a line is selected, 1 if no lines were
           selected, and 2 if an error occurred.  However, if the -q or --quiet or
           --silent is used and a line is selected, the exit status is 0  even  if
           an error occurred
    
  • GNU tar(1) uses a similar scheme:

    RETURN VALUE
           Tar exit code indicates whether it was able to successfully perform the
           requested operation, and if not, what kind of error occurred.
    
           0      Successful termination.
    
           1      Some  files  differ.   If  tar  was  invoked  with the --compare
                  (--diff, -d) command line option, this means that some files  in
                  the  archive  differ  from  their disk counterparts.  If tar was
                  given one of the --create, --append or  --update  options,  this
                  exit  code  means  that  some  files  were  changed  while being
                  archived and so the resulting archive does not contain the exact
                  copy of the file set.
    
           2      Fatal  error.   This  means that some fatal, unrecoverable error
                  occurred.
    
  • PostgreSQL's psql command uses four exit status code: 0-4. From psql(1):

    EXIT STATUS
           psql returns 0 to the shell if it finished normally, 1 if a fatal error
           of its own occurs (e.g. out of memory, file not found), 2 if the
           connection to the server went bad and the session was not interactive,
           and 3 if an error occurred in a script and the variable ON_ERROR_STOP
           was set.
    
  • With gzip we're back to the three codes: 0, 1, and 2. From gzip(1):

    DIAGNOSTICS
           Exit status is normally 0; if an error occurs, exit status is 1.  If  a
           warning occurs, exit status is 2.
    
  • GNU wget(1) uses more codes than most, but still fewer than 10:

    EXIT STATUS
           Wget may return one of several error codes if it encounters problems.
    
           0   No problems occurred.
    
           1   Generic error code.
    
           2   Parse error---for instance, when parsing command-line options, the
               .wgetrc or .netrc...
    
           3   File I/O error.
    
           4   Network failure.
    
           5   SSL verification failure.
    
           6   Username/password authentication failure.
    
           7   Protocol errors.
    
           8   Server issued an error response.
    
           With the exceptions of 0 and 1, the lower-numbered exit codes take
           precedence over higher-numbered ones, when multiple types of errors are
           encountered.
    
  • fetchmail(1) is somewhere in the middle. It documents the use of 29 non-zero status codes. That's quite a few, but a lot of those are things somebody scripting the tool might want to deal with specifcially in different circumstances.

  • curl(1) takes an entirely different approach: it goes the hole hog conveying detailed information via its exit codes -- too many to list here, but curl(1) from version 7.72.0 enumerates 96 specific errors, and promises more to come. Considering 125 as the highest value that should be explicitly used by a user program, it feels a bit like curl is on its way to running out of slots for new exit statuses!

  • Entry for sqsh(1) elided because, while it is well-intended, I do not think it is a good example to emulate :-)

Maybe we should open a separate issue for a new "check" type of tool, and move this part of the discussion to it? See issue #872

@abonander
Copy link
Collaborator

It seems superfluous to have basically the same subcommand just under different names and one changes its exit code based on migration status.

@salewski
Copy link
Contributor Author

salewski commented Dec 3, 2020

It seems superfluous to have basically the same subcommand just under different names and one changes its exit code based on migration status.

@abonander Yeah, I agree, if that would be the only difference. But if that's the case, then I misunderstood your intent. I guess I was reading too much into the word "check" in sqlx migrate check, b/c I got the impression that you were proposing a new tool that would do something deeper than detect/report on the high-level "pending"/"installed"/"installed-but-changed" states.

Apologies for the noise.

@nixpulvis
Copy link

nixpulvis commented Feb 23, 2021

So let me just ask. Is there a plan to support some kind of command to help fix this issue? As a new user of this library, I just got stuck on this. I think one thing that would help me personally is a way to re-validate a migration. Here's my current situation:

  1. I created a migration with sqlx migrate add ...
  2. I ran the migration with sqlx run
  3. I then imported a bunch of test data
  4. Then I ended up "playing around" with things, since I'm developing the migration itself and I needed to add an index and enable an extension, etc
  5. Finally, I'm creating a new migration now...

I don't really want to drop my database (or rollback) to re-run the migration, since I already went through the effort of adding the test data.

What I'd like to be able to do is update the checksum (and maybe installed_on / execution_time), to confirm the state I'm in matches the migration. Or if that's too hard, maybe just an easy way to generate the checksum myself so I can update the column.

Basically, I think this use case should be considered. I'm capable of working around it, but it's going to happen again and again.

EDIT:

Just in case anyone else finds themselves in this situation, here's the workaround:

cat migrations/<name>.sql | openssl dgst -sha384 | cut -d ' ' -f 2
UPDATE _sqlx_migrations SET checksum = '\x<hash>' WHERE version = <version>;

@brianbruggeman
Copy link

I think this is kind of broken for development, especially where the developer is unfamiliar with the cli interface and the expected workflow and making mistakes (or at least poking around and breaking things).

Somewhere, as a developer, we would need a full CRUD interface where we can apply, re-apply, update, rollback and remove.

It's also completely unclear (and a little surprising/irritating) that there was an addition made to the database. While I think the database is a nice way to control migrations, I may not want those in my database.

While developing, I'm definitely going to end up running this quite a bit:

DROP TABLE IF EXISTS _sqlx_migrations

Ideally, for the issue @nixpulvis mentioned, we'd have a CLI for updating the checksum.

@rubenfiszel
Copy link

I wrote a little migration script to overwrite all migrations for the need of: https://github.com/windmill-labs/windmill

I thought it could be useful to anyone reading this thread:

import sys
import os
import subprocess

database_url = sys.argv[1]

print(f"database_url: {database_url}")

if database_url is None:
    print("Please provide a database url")
    sys.exit(1)

for f in os.listdir("migrations"):
    if f.endswith(".up.sql"):
        version = f.split("_")[0]
        cmd = f"cat migrations/{f} | openssl dgst -sha384 | cut -d ' ' -f 2"
        ps = subprocess.Popen(cmd,shell=True,stdout=subprocess.PIPE,stderr=subprocess.STDOUT)
        digest = ps.communicate()[0].decode("utf-8").strip()

        cmd = f"psql '{database_url}' -c \"UPDATE _sqlx_migrations SET checksum = '\\x{digest}' WHERE version = {version};\""
        ps = subprocess.Popen(cmd,shell=True,stdout=subprocess.PIPE,stderr=subprocess.STDOUT)
        out = ps.communicate()[0].decode("utf-8").strip()
        print(version, digest, out)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Concerns `sqlx-cli` E-easy enhancement New feature or request migrations Proposals or bugs involving migrations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants