Change this_script_should_run() logic #35

Merged
merged 1 commit into from Jan 2, 2012

Conversation

Projects
None yet
4 participants
@BiggerNoise
Member

BiggerNoise commented Nov 27, 2011

So that one time scripts with changes are executed
(warn / error) is handled elsewhere

Corrects: #32

Change logic to allow one time scripts with changes to
execute (warn / error) is handled elsewhere
@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Jan 1, 2012

Member

Hey guys, before I merge this, can I get a confirmation that this change in behavior is what we want.

This relates to an altered one time script

By default, RH reports this as an error and stops execution. No problems there.

Currently, if you set "warnononetimescriptchanges" true, RH will report the altered script and continue running. It will not attempt to execute the updated script.

This patch changes that behavior so that if you set "warnononetimescriptchanges" true, RH reports script alteration, then runs the updated script and records the new script run information. I think this is far more useful, and I gather from my conversations with Rob, it was the expected behavior.

If you have an objection to me merging this in, please give a shout.

Member

BiggerNoise commented Jan 1, 2012

Hey guys, before I merge this, can I get a confirmation that this change in behavior is what we want.

This relates to an altered one time script

By default, RH reports this as an error and stops execution. No problems there.

Currently, if you set "warnononetimescriptchanges" true, RH will report the altered script and continue running. It will not attempt to execute the updated script.

This patch changes that behavior so that if you set "warnononetimescriptchanges" true, RH reports script alteration, then runs the updated script and records the new script run information. I think this is far more useful, and I gather from my conversations with Rob, it was the expected behavior.

If you have an objection to me merging this in, please give a shout.

BiggerNoise added a commit that referenced this pull request Jan 2, 2012

Merge pull request #35 from BiggerNoise/topic/warnononetimescriptchan…
…ges_correction

Change  this_script_should_run() logic

@BiggerNoise BiggerNoise merged commit f333dcf into chucknorris:master Jan 2, 2012

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

I was originally in favor of this change, but after some experience with roundhouse in production, I think this is a bad idea. At the very least the options should be: warn, warnAndExecute, and error. Actually, if you want the re-run behavior you can always rename the script, so warnAndExecute is really redundant and forcing the re-execution of a script is actually taking away functionality.

There have been times where I have changed an older script so that it is cleaner/faster when initializing a new database. If these scripts are now re-run on a product system, they could have unintended side-effects. By removing the option to warn without execute, it will no longer be possible to make changes that are only intended to affect new installs, and are not intended to be run against up-to-date databases.

Just my 2 cents.

jonreis commented Jul 25, 2013

I was originally in favor of this change, but after some experience with roundhouse in production, I think this is a bad idea. At the very least the options should be: warn, warnAndExecute, and error. Actually, if you want the re-run behavior you can always rename the script, so warnAndExecute is really redundant and forcing the re-execution of a script is actually taking away functionality.

There have been times where I have changed an older script so that it is cleaner/faster when initializing a new database. If these scripts are now re-run on a product system, they could have unintended side-effects. By removing the option to warn without execute, it will no longer be possible to make changes that are only intended to affect new installs, and are not intended to be run against up-to-date databases.

Just my 2 cents.

@mpareja

This comment has been minimized.

Show comment
Hide comment
@mpareja

mpareja Jul 25, 2013

Member

You make a great point.

Also, the name used for this parameter is unclear. Perhaps we should introduce a new parameter (like ChangedOneTimeScriptAction) with the values you mentioned and obsolete (i.e. remove from docs but secretly support) the existing parameter. Ideally, the obsolete parameter would simply set the value of ChangedOneTimeScriptAction appropriately.

Thoughts?

Member

mpareja commented Jul 25, 2013

You make a great point.

Also, the name used for this parameter is unclear. Perhaps we should introduce a new parameter (like ChangedOneTimeScriptAction) with the values you mentioned and obsolete (i.e. remove from docs but secretly support) the existing parameter. Ideally, the obsolete parameter would simply set the value of ChangedOneTimeScriptAction appropriately.

Thoughts?

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

Actually I'm not sure this came out the way it was intended. It should only run if it has changed AND you have warnononetimescriptchanges set to true. But I agree, there should be a warn and execute changed scripts to take away the ambiguity.

Member

ferventcoder commented Jul 25, 2013

Actually I'm not sure this came out the way it was intended. It should only run if it has changed AND you have warnononetimescriptchanges set to true. But I agree, there should be a warn and execute changed scripts to take away the ambiguity.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

Actually nevermind on whether it executes, I think that is handled elsewhere.

Member

ferventcoder commented Jul 25, 2013

Actually nevermind on whether it executes, I think that is handled elsewhere.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

Hello Mario, I think your idea makes sense.

I looked back through some of the history, and it seems that I logged this issue a year ago, and lost the argument. See: #71

Not sure what the reluctance is to allow the choice of whether to warn, warn and execute, or error is?

jonreis commented Jul 25, 2013

Hello Mario, I think your idea makes sense.

I looked back through some of the history, and it seems that I logged this issue a year ago, and lost the argument. See: #71

Not sure what the reluctance is to allow the choice of whether to warn, warn and execute, or error is?

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

I kind of teetered on this argument back and forth, but it kind of makes sense that folks would want to run the script if it has changed in some cases. There is no reluctance to make it a little more specific like /warn /executechanged

Member

ferventcoder commented Jul 25, 2013

I kind of teetered on this argument back and forth, but it kind of makes sense that folks would want to run the script if it has changed in some cases. There is no reluctance to make it a little more specific like /warn /executechanged

@mpareja

This comment has been minimized.

Show comment
Hide comment
@mpareja

mpareja Jul 25, 2013

Member

It sounds to me like you're going to be running deployments against your existing production databases with the option set to "warn" at all times. That might make me a bit uneasy because it wouldn't be clear what script changes were intended and which weren't.

Member

mpareja commented Jul 25, 2013

It sounds to me like you're going to be running deployments against your existing production databases with the option set to "warn" at all times. That might make me a bit uneasy because it wouldn't be clear what script changes were intended and which weren't.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

Yes, we run our scripts from an installer with warn=true. When we want a script to be re-run, we change its name.

jonreis commented Jul 25, 2013

Yes, we run our scripts from an installer with warn=true. When we want a script to be re-run, we change its name.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

Why would you do this? Is there something broken in RH that makes you need to do this? Using /warn should be an exceptional case, not something you normally roll with.

Member

ferventcoder commented Jul 25, 2013

Why would you do this? Is there something broken in RH that makes you need to do this? Using /warn should be an exceptional case, not something you normally roll with.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

No, not at all, but I do think the developer should be in control of when a ONE TIME script gets re-run, and when it doesn't. I don't think RH should make this decision for me.

When I check in a ONE TIME script with changes that are intended to be re-run, I can rename the script and it will re-run. If I change an older ONE TIME script, for example to change whitespace, add a comment, or perhaps remove code that new databases installations can skip, then it will not be re-run. I like to be in control of this behavior.

jonreis commented Jul 25, 2013

No, not at all, but I do think the developer should be in control of when a ONE TIME script gets re-run, and when it doesn't. I don't think RH should make this decision for me.

When I check in a ONE TIME script with changes that are intended to be re-run, I can rename the script and it will re-run. If I change an older ONE TIME script, for example to change whitespace, add a comment, or perhaps remove code that new databases installations can skip, then it will not be re-run. I like to be in control of this behavior.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

You are talking about a script in a ONE time folder. In the other folders it will run without any change to the name.

Member

ferventcoder commented Jul 25, 2013

You are talking about a script in a ONE time folder. In the other folders it will run without any change to the name.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

Yes, the one-time folder.

jonreis commented Jul 25, 2013

Yes, the one-time folder.

@mpareja

This comment has been minimized.

Show comment
Hide comment
@mpareja

mpareja Jul 25, 2013

Member

Regarding your optimizations for generating a database from scratch: could you just put it together in the runAfterCreateDatabase scripts and remove the old version from the Up directory? Or do you have multiple databases at different versions for which you would still need the up script?

Member

mpareja commented Jul 25, 2013

Regarding your optimizations for generating a database from scratch: could you just put it together in the runAfterCreateDatabase scripts and remove the old version from the Up directory? Or do you have multiple databases at different versions for which you would still need the up script?

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

The way you are working the up folder is not the way it was designed to be used. The scripts in here freeze when you run it against a machine that can not drop and rebuild the database or start from a restore of a backup (usually production).

Every time you want to change something, it's a new script. Removing things that no longer apply should happen in a script later that would remove it.

The way you are going about it now seems like a lot of maintenance to the up folder that frankly doesn't seem necessary. I'm open to understanding that this is necessary in your case.

Also, there is a runAfterCreate folder where you can archive scripts that you no longer want to see (also a one time folder) but have run when a database is created.

Member

ferventcoder commented Jul 25, 2013

The way you are working the up folder is not the way it was designed to be used. The scripts in here freeze when you run it against a machine that can not drop and rebuild the database or start from a restore of a backup (usually production).

Every time you want to change something, it's a new script. Removing things that no longer apply should happen in a script later that would remove it.

The way you are going about it now seems like a lot of maintenance to the up folder that frankly doesn't seem necessary. I'm open to understanding that this is necessary in your case.

Also, there is a runAfterCreate folder where you can archive scripts that you no longer want to see (also a one time folder) but have run when a database is created.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

I understand changing a ONE TIME script is a bad thing, but sometimes you want to make a change because when you checked it in, you forgot a scenario that breaks on a new installation (maybe an upgrade to a new version of sql server). All you want to do is make the change so that new installs don't fail. Existing installs are ok so you don't want them to be force to run the script, but you don't want them to error either.

So what do you recommend if a one-time script needs to be changed because an issue was found post release that affects some, but not all systems?

jonreis commented Jul 25, 2013

I understand changing a ONE TIME script is a bad thing, but sometimes you want to make a change because when you checked it in, you forgot a scenario that breaks on a new installation (maybe an upgrade to a new version of sql server). All you want to do is make the change so that new installs don't fail. Existing installs are ok so you don't want them to be force to run the script, but you don't want them to error either.

So what do you recommend if a one-time script needs to be changed because an issue was found post release that affects some, but not all systems?

@mpareja

This comment has been minimized.

Show comment
Hide comment
@mpareja

mpareja Jul 25, 2013

Member

Using the warn flag to accomplish this in general seems like a sledgehammer approach that can go wrong (a change didn't get deployed and instead you were quietly warned).

I know RH saves you from doing the following in general, but perhaps a better solution for you would simply be to add a new script which is smart enough to determine whether changes need to be made. This shouldn't be something you're doing very often at all.

Member

mpareja commented Jul 25, 2013

Using the warn flag to accomplish this in general seems like a sledgehammer approach that can go wrong (a change didn't get deployed and instead you were quietly warned).

I know RH saves you from doing the following in general, but perhaps a better solution for you would simply be to add a new script which is smart enough to determine whether changes need to be made. This shouldn't be something you're doing very often at all.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

When you put it in the runAfterCreateDatabase folder, you can change it as often you want. Because it will ALWAYS be the first time that the script has ever run against a database.

That folder is not evaluated on an existing database. It sounds like that workflow will work much better for your needs.

Member

ferventcoder commented Jul 25, 2013

When you put it in the runAfterCreateDatabase folder, you can change it as often you want. Because it will ALWAYS be the first time that the script has ever run against a database.

That folder is not evaluated on an existing database. It sounds like that workflow will work much better for your needs.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

Yes, sometimes you need a sledgehammer, and when you do, you are sure glad to have it.

When you put it in the runAfterCreateDatabase folder, you can change it as often you want. Because it will ALWAYS be the first time that the script has ever run against a database.

The point here is that it is a ONE-TIME script with a bug. It is not something you want to run the next time a production system sees it. It sounds like you should never have a bug in a previously run ONE-TIME script ;).

At any rate, it looks like I will just have to change the way I use RH. Not a problem. Thanks for listening.

jonreis commented Jul 25, 2013

Yes, sometimes you need a sledgehammer, and when you do, you are sure glad to have it.

When you put it in the runAfterCreateDatabase folder, you can change it as often you want. Because it will ALWAYS be the first time that the script has ever run against a database.

The point here is that it is a ONE-TIME script with a bug. It is not something you want to run the next time a production system sees it. It sounds like you should never have a bug in a previously run ONE-TIME script ;).

At any rate, it looks like I will just have to change the way I use RH. Not a problem. Thanks for listening.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
Member

ferventcoder commented Jul 25, 2013

:)

@mpareja

This comment has been minimized.

Show comment
Hide comment
@mpareja

mpareja Jul 25, 2013

Member

I appreciate you taking part in this conversation. We're just trying to get a good understanding of the need so we can offer the "right" functionality.

I'm still of the opinion that the name of the current flag is wrong given that the scripts are re-executed upon giving the warning.

Member

mpareja commented Jul 25, 2013

I appreciate you taking part in this conversation. We're just trying to get a good understanding of the need so we can offer the "right" functionality.

I'm still of the opinion that the name of the current flag is wrong given that the scripts are re-executed upon giving the warning.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Jul 25, 2013

Member

I agree with @mpareja on this. I think we should have an additional flag that can be used to execute changes on one time scripts. Let's file another issue and reference this issue.

Member

ferventcoder commented Jul 25, 2013

I agree with @mpareja on this. I think we should have an additional flag that can be used to execute changes on one time scripts. Let's file another issue and reference this issue.

@jonreis

This comment has been minimized.

Show comment
Hide comment
@jonreis

jonreis Jul 25, 2013

So the plan is to:

  1. ALWAYS execute ONE-TIME scripts when warnononetimescriptchanges=true
  2. ALWAYS error and stop processing when warnononetimescriptchanges=false

Rob, what is this other option you are proposing? How is affected by the warnononetimescriptchanges setting?

If I haven't said it before, thank you guys for making such a great library.

jonreis commented Jul 25, 2013

So the plan is to:

  1. ALWAYS execute ONE-TIME scripts when warnononetimescriptchanges=true
  2. ALWAYS error and stop processing when warnononetimescriptchanges=false

Rob, what is this other option you are proposing? How is affected by the warnononetimescriptchanges setting?

If I haven't said it before, thank you guys for making such a great library.

@mpareja

This comment has been minimized.

Show comment
Hide comment
@mpareja

mpareja Jul 25, 2013

Member

The behaviour currently in 0.8.6 is basically what @jonreis said, but I'll restate for clarity since I just updated this in the documentation:

  1. WarnOnOneTimeScriptChanges = true: Instructs RH to execute changed one time scripts that have previously been run against the database. A warning is logged for each one time scripts that is rerun.
  2. WarnOnOneTimeScriptChanges = false: Instructs RH to error and stop processing upon encountering a one time script that was previously run against a database and has since changed.

The impression I get when I read "warnOnOneTimeScriptChange" is that RH is going to do something more (issue a warning) rather than less (not issue an error). I think this option name should be improved.

This leaves us with two options:

  1. If we were to add the third state that @jonreis lobbied for (warn but skip), we should go with something like the proposed "ChangedOneTimeScriptAction". I'm not saying whether I do or don't think we should support this, I'm just giving the options.
  2. If we don't add the third state, then perhaps renaming that flag to be "rerunChangedOneTimeScripts" would make it more obvious.
Member

mpareja commented Jul 25, 2013

The behaviour currently in 0.8.6 is basically what @jonreis said, but I'll restate for clarity since I just updated this in the documentation:

  1. WarnOnOneTimeScriptChanges = true: Instructs RH to execute changed one time scripts that have previously been run against the database. A warning is logged for each one time scripts that is rerun.
  2. WarnOnOneTimeScriptChanges = false: Instructs RH to error and stop processing upon encountering a one time script that was previously run against a database and has since changed.

The impression I get when I read "warnOnOneTimeScriptChange" is that RH is going to do something more (issue a warning) rather than less (not issue an error). I think this option name should be improved.

This leaves us with two options:

  1. If we were to add the third state that @jonreis lobbied for (warn but skip), we should go with something like the proposed "ChangedOneTimeScriptAction". I'm not saying whether I do or don't think we should support this, I'm just giving the options.
  2. If we don't add the third state, then perhaps renaming that flag to be "rerunChangedOneTimeScripts" would make it more obvious.

GoogleCodeExporter pushed a commit to google-code-export/roundhouse that referenced this pull request Mar 13, 2015

ferventcoder
Enhancement - Allow for one time script to run (chucknorris/roundhous…
…e#35). Fix - Don't run scripts that have already been run and not modified (chucknorris/roundhouse#42)

ChrisMissal pushed a commit to ChrisMissal/roundhouse that referenced this pull request Jul 30, 2015

Enhancement - Allow for one time script to run (chucknorris/roundhous…
…e#35). Fix - Don't run scripts that have already been run and not modified (chucknorris/roundhouse#42)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment