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

dev/release#16 - Cleanup empty upgrade steps #19744

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

totten
Copy link
Member

@totten totten commented Mar 6, 2021

Overview

Streamline the upgrader by removing a number of empty upgrade steps.

This patch is blocked on the dependency #19743 (which accounts for the first half-dozen commits). It furthers https://lab.civicrm.org/dev/release/-/issues/16.

ping @colemanw

Technical Details

Here was my process:

  • cd into CRM/Upgrade/Incremental/sql
  • Run ls -l on subset of files
  • Make a list of files that were 55-63 bytes long. Display them and confirm that they contain no real instructions.
  • Take the list of removed versions. Grep to see if those versions have PHP functions.
  • If there is a PHP function, it may have a runSql() step that needs to be disabled.

In reviewing the diffs, you should generally see two kinds of changes:

  • Several *.mysql.tpl files are removed -- all of these empty, notwithstanding boilerplate comments.
  • In some of the upgrade_X_Y_Z() functions, calls to runSql() are commented out. That's because the identified files no longer exist. (One can verify they don't exist by skimming, eg, ls CRM/Upgrade/Incremental/sql/ | sort | grep ^5)

@civibot
Copy link

civibot bot commented Mar 6, 2021

(Standard links)

@civibot civibot bot added the master label Mar 6, 2021
@eileenmcnaughton
Copy link
Contributor

@totten this needs rebasing now - but it also did not pass my tests - ie I used the sql from the 4.5 tarball and then ran the upgrade & it failed on a missing sql file

@eileenmcnaughton
Copy link
Contributor

I'm pretty sure the issue I hit would also be covered by the unit tests TBH

@eileenmcnaughton eileenmcnaughton changed the title (Blocked) dev/release#16 - Cleanup empty upgrade steps dev/release#16 - Cleanup empty upgrade steps Apr 7, 2021
@eileenmcnaughton
Copy link
Contributor

Not blocked anymore - just needs fixing up

@totten totten force-pushed the master-upg-cleanup branch from 63f3b19 to 50c5323 Compare April 8, 2021 02:12
@totten
Copy link
Member Author

totten commented Apr 8, 2021

@eileenmcnaughton Thanks for that ping.

I've rebased now. Additionally, for testing, I used this procedure*

  • Load BUILDKIT/vendor/civicrm/upgrade-test/databases/4.7.31-setupsh.sql.bz2
  • Run existing upgrader and take DB snapshot
  • Load BUILDKIT/vendor/civicrm/upgrade-test/databases/4.7.31-setupsh.sql.bz2
  • Run modified upgrader and take DB snapshot

Aside from the civicrm_cache and civicrm_log (timestamps), they appeared the same.

@eileenmcnaughton
Copy link
Contributor

I don't think I need to re-do those same test steps - I did something similar on the other PR - if this passes let's merge it

@monishdeb
Copy link
Member

Merging it as per MOP tag

@monishdeb monishdeb merged commit 39be8a3 into civicrm:master Apr 8, 2021
@totten totten deleted the master-upg-cleanup branch April 9, 2021 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants