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

Prepping 3.3.0 upgrade #71

Merged
merged 12 commits into from Feb 6, 2019
Merged

Prepping 3.3.0 upgrade #71

merged 12 commits into from Feb 6, 2019

Conversation

mattbrailsford
Copy link
Contributor

Added upgrade code to delete old payment provider 3rd party dependencies

@mattbrailsford
Copy link
Contributor Author

@andersburla I'm prepping an update as I've updated the payment providers with the latest stripe and also implemented using ILMerge to merge payment provider dependencies. Because of this, I'm doing an upgrade step to delete the old 3rd party dlls (as these are now merged into the core PaymentProvide dll).

I'm happy with how I have this setup, but I'd quite like your feedback regarding the upgrade steps. It occurred to me whilst preparing this that the upgrade steps only work sequentially meaning you would have to upgrade to each intermediary SpecialActionsVersion manually in order for the upgrades to occur as the upgrades are occurring only if the currentVersion is the previous version number. I'm wondering whether these should actually be updated to use < rather than == when checking current version numbers, ie

if (currentVersion < 4) {
    // Do something...
}

As this would then allow say someone on an install of 3 to jump up to version 5 and have all the upgrade steps in between run in one go. Like I say, with the current == approach, if the dev is on version 3, they would have to upgrades to v4 and then upgrade again to v5. Does that make sense? Does it sounds like using < would be the actual desired approach?

@andersburla
Copy link
Contributor

The previous code did run sequentially and could do multiple updates in one go. If you had special version 3 and the code said newVersion = 6 - it would do 4 + 5 + 6 as its a while loop. For each loop it updates the specialActionsVersion in the database - so if step 5 failed - it would still have done the version 4 update. Then you could try and see in the log what the error was, and then boot your application again to try and run 5 + 6 again.

At the moment the code will fail because currentVersion variable is never changed and it would be an infinite loop. Line 187 you removed - was doing this before. You should add 1 to the currentVersion variable when you do the update of SpecialActionsVersion in the DB in line 225.

The reason to use currentVersion + 1 == 6 is to have each while loop only execute one of the version codes. Then it would update the version number in the DB and then run the next version update in the while loop. If you do currentVersion < 4 and also have < 5 and < 6 in two other if statements, then they would also run in the same while loop iteration. If 5 then fails - then your DB won't be updated with the SpecialActionsVersion to 4, and when the system boots, it will try and do version 4 again - which could then fail, if they code didn't support this case. So we did it the current way to allow each special version to run separately and update the SpecialActionsVersion in the DB for each while loop iteration.

@mattbrailsford
Copy link
Contributor Author

@andersburla ahh, of course. I missed the fact it was in a while loop. That makes total sense now. I have re-instated the loop counter so should be back to how it was before. Thanks for the feedback.

@mattbrailsford
Copy link
Contributor Author

@andersburla I've introduced the concept of a "stepTargetVersion" which effectively stores the target version of the current loop. I think this makes the if statements a little easier to read, rather than doing + 1 math. This should pretty much be as before, but I'm wondering why

  1. Step 2.1.0 runs on stepTargetVersion 1, which is essentially a fresh install. Is this because the version number table wasn't a thing before?

  2. I've checked the current version and the database table SpecialActionsVersion is set to 5 so I've set this target version to 6, however I notice there are not special actions for version 5 so I'm wondering why the number was ever increased to 5?

@andersburla
Copy link
Contributor

  1. When installing TC the SpecialActionsVersion will be 0. So each upgrade step will run - also for a clean install where the things might have been fixed. That is why each upgrade proces should take into account that things might be fixed and files are not there anymore. Maybe it could have been done smarter - but that is how it was done. Can't remember if we had the SpecialActionsVersion in DB for version 2.0 which was built from scratch.

  2. Yeah - can't remember what happend to v5. Might have been a thing that wasn't needed anymore. Try see the history for the .cs file and see when it was removed. But it doesn't matter those numbers. Just what happens at some specific upgrade versions. You could change the target to 10 without anything happening. Only when you use the version number to something in the code that it matters.

@mattbrailsford mattbrailsford changed the title Prepping 3.2.5 upgrade Prepping 3.2.6 upgrade Feb 5, 2019
@mattbrailsford mattbrailsford added this to the 3.2.6 milestone Feb 5, 2019
@mattbrailsford mattbrailsford changed the title Prepping 3.2.6 upgrade Prepping 3.1.0 upgrade Feb 6, 2019
@mattbrailsford mattbrailsford changed the title Prepping 3.1.0 upgrade Prepping 3.2.6 upgrade Feb 6, 2019
@mattbrailsford mattbrailsford changed the title Prepping 3.2.6 upgrade Prepping 3.3.0 upgrade Feb 6, 2019
@mattbrailsford mattbrailsford merged commit 0cca551 into dev Feb 6, 2019
@mattbrailsford mattbrailsford deleted the wip/3.2.5-upgrade branch February 6, 2019 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants