warnononetimescriptchanges does not behave as expected

[BiggerNoise]

I would expect that when I enable that option I would:

• get a warning
• the script would be executed
• the hash code would be updated in the database

Instead, I:

• get a warning
• an indication that the script was skipped

[leblancmeneses]

+1 -w does not behave correctly.

Did hash calculation change? - reviewing history of file there hasn't been any changes. Maybe an encoding change.

[BiggerNoise]

I have fixed this issue, we just have not done a release since that fix was committed. If you build RH from source, you should see the expected behavior that I described.

[leblancmeneses]

The console does not highlight the warning in yellow - although it is behaving as you mentioned.

I did like the previous behavior of skipping.

Currently I'm getting a lot of these reexecution attempts on insert data and old scripts that haven't been touched in months. They are part of a Merge operation but TFS says it is identical. However, the calculated hash in roundhouse must now be different on these specific files. If you change a script already pushed to a staging environment you should run a dbrestore operation on the staging environment to reverse a bad script committed. - re-executing could cause more problems than skipping.

I changed my copy of source to skip...

[jonreis]

I think the old behavior was much better. Before you had a choice, if you wanted the script to re-run, you just had to change the name of the script. Now the scripts are forced to run. This will definitely cause problems with existing installs.

[ferventcoder]

@leblancmeneses It's possibly an encoding change. It could also be a line ending format change. The other is that you were using ANSI and the code page (I think that is what it is called) on your computer changed.

Always best to have your scripts be UTF-8.

[ferventcoder]

Is this still an issue?

[leblancmeneses]

On a new project i think it would work fine but on existing apps like ours the new behavior caused problems.
I modified source so -w behaved like the original rh.exe we were using. - no longer an issue.

[ferventcoder]

Maybe we need a new switch to skip modified up scripts?

[leblancmeneses]

well the current flag says:-w = warnononetimescriptchanges - it doesn't describe other actions it completes (execute or skip)

new possible flags:
-we = warnandexecuteononetimescriptchanges
-ws = warnandskipononetimescriptchanges

i prefer -ws as in older versions of rh. the new version of rh would use -we

[jonreis]

Does skip update the hash as if it were run? It probably has to or you will always have to run with a -w option.

[BiggerNoise]

One thing that might be worth considering is adding some of these off-axis things as command arguments. Similar to the way that git command line works git command [options], we could have:

rh migrate [options for migrate]
rh repair [options for repair]
etc...

(migrate could be the default command so we didn't suddenly break all of our existing users.)

The thing I like about this is that it separates migration from stuff you should really pay attention to. It also at least attempts a taxonomy of flags, of which, we have quite a few.

This is just a notion that's barely stirred (let alone half-baked ) but curious what all ya'lls think about it.

[ferventcoder]

Might be an interesting concept. I'm not on board yet but I'm open to hearing where you are going with this.

[kyberias]

It's amazing how you've managed to break the tool by reversing an option that actually would have allowed users to ignore any changed made to scripts.

[ferventcoder]

@kyberias can you elaborate?

[kyberias]

Certainly. If I've understood correctly, the original version stopped on changed up-scripts and gave a warning and the -w option disabled the warning and skipped the changed files. This sounds useful. Now you've reversed the functionality of the option without providing the original functionality (skip the files). Now it's impossible to skip the changed files. Correct me if I'm wrong.

[BiggerNoise]

The problem with the previous behavior is that there's no way to exit the situation. A warning is a situation that you might tolerate briefly if you had much more important fish to fry, but creating a situation where the software must always allow warnings and you must ignore them forever is not good engineering practice.

Roundhouse is trying to warn you of something that should be dealt with, not something that should be ignored.

Why not deal with it by altering your offending script to be idempotent. That is, have it detect if the changes to be made are already present. If so, skip the body of the script. Then you get the warning once, but the script succeeds because the body is skipped.

A few times of that and you'll start getting good at not modifying up scripts.

[kyberias]

I agree that good engineering practices are important. Let me try to explain what happened here:

1. You've changed existing behaviour by reversing it. This is never a good idea. Someone might have depended on this behaviour for whatever reason and now might end up surprisingly executing SQL in the database.
2. The current documented semantics of the up-folder are this: "scripts are run once in a given order." Introducing a switch that runs any modified scripts again changes this semantics to something like this: "run these scripts multiple times (out of order) whenever someone changes it".

Since these are one-time scripts and we all agree it's a bad idea to modify one-time scripts, there is no reason to change the tool to support that. It's very confusing and potentially dangerous.

[kyberias]

The reason I'm being critical here is because I'm using the tool in production and want to understand what is the probability of roundhouse changing semantics in surprising ways in the future and break my databases. :)

[BiggerNoise]

Scripts are run once; your altered script has never been run. When you change an up script you make the up directory ambiguous.

The behavior was changed to make it correspond to the documented behavior for the flag. Further, the original behavior was pathologically bad: Once a script was altered anywhere, you had no choice but to turn on the warnings flag and ignore all future changes to the script or any other script that might change.

Altered up scripts are a serious issue in your code base and should be remediated.

I'm not going to discuss any further why the change is a good one; we beat this to death two years ago.

Finally, your lamentable conduct is neither constructive nor professional: You can ask why something changed without being a jerk (although, it might help to actually read a bit about the change first).

After taking time to understand why the change was made (even if you don't like it), you could offer a pull request that solved the problem in a better way.

[leblancmeneses]

I didn't agree it was a good idea either at the time. I still don't so I pulled source and modified the exe to my and my client needs.

[kyberias]

I know why you changed it, I don't need to ask. Now I was trying to find out whether you could understand the severity of the consequences and perhaps revert the absurd change. Despite the feedback from multiple people (above), it is clear to me that is not going to happen. After the discussion and disagreement, it's hard for me to believe that any pull requests regarding this functionality would be respected. No hard feelings!

[BiggerNoise]

This discussion does not capture the full discussion we had at the time. I will briefly summarize the rationale for the change so we don't have to do this again in another two years.

1. Bob creates an up script that creates a users table and give it a first_name and last_name field
2. Bob commits this script to source control.
3. A week later, Bob realizes that he needs an SSN field and (incorrectly) adds it to the create table script.
4. Bob commits his change to source control

With the old behavior, if you had the warning flag enabled (as you would eventually have to), you would see a warning, but the table would not have the SSN column. This has the following consequences:

• The database no longer corresponds to the commit. It is dependent upon how you got to the commit
• When up scripts are changed, the only way that you know about it is that at some point code that depends on the change will not work.
• There's no immediate feedback mechanism to let Bob know that he has done something wrong.

The old behavior is the equivalent of dealing with a failing unit test by changing the settings on your CI server so that failing unit tests no longer break the build. It really and truly was a bug, it was never intended to behave as it did.

With the new behavior, you do not normally run with the flag enabled. Now when Bob alters the up script, your CI server fails the build. Bob, perhaps with some help, remediates the script to deal with the possible situations: The table does not exist, the table exists but has no ssn column, or the table exists with the ssn column. The create users script becomes idempotent and always ends with the table correctly having the ssn column.

Now anyone that has run either of the first two versions of the script has to run roundhouse with the flag set once. This will remediate their database. Subsequent runs are performed without the flag enabled.

This situation is still not perfect, but it does have the following consequences:

• Once past the remediation commit, the databases are identical
• You know about one time script changes immediately because they break the build. This is a good thing, the whole point of RH is to (as much as possible) treat your database like code. Broken code breaks the build, so should a broken database.
• Bob quickly learns to not alter up scripts because he immediately has to deal with the breakage of doing so.

The ssn example is chosen for starkness and brevity. There are a lot of very subtle changes that could be made (field widths, nullability, default value constraints, indexes, etc.) that could yield very different behavior.

[ferventcoder]

I think the end state of this conversation is that we need an additional flag that allows those items to run. I can't say I understand or agree with everyone's methodologies on database changes. We have flags in for many things for precisely this reason. It allows folks to shape the tool in a way that works for them. TBH, I know situations where there are issues with something like using ANSI code page and those items really have not changed, but for some reason the tool thinks they have. In those situations you could not use RH with the current way it works alone. That's why I believe an additional flag is warranted to allow a bypass of "changed" scripts.

I also agree with Andy, you don't want to be in the -w forever either. It's not a good place to be and should really not be used when you get to production. You should have tested many times prior to that and found and fixed the issue prior to hitting production.

[ferventcoder]

I was ambiguous a bit there. -syncOneTimeChanges is probably more likely

[BiggerNoise]

I know I never fleshed out what I was talking about with the rh migrate/repair; but I think it would be great to have RH always return an error if it thinks the script has changed.

Then have a separate utility, rh_repar (or something like that), that allows you to decide what to do about scripts that RH thinks have changed. It would allow you to update the hash with or without running the script.

I hate to keep piling flags on; I think it leads to flags that should be used for exceptional situations becoming part of the everyday workflow.