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.

Sign up for GitHub

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

Jump to bottom

Remove upgrade scripts in favor of autoupgrade process #25794

Merged
merged 3 commits into from Oct 26, 2021
Merged

Remove upgrade scripts in favor of autoupgrade process #25794

merged 3 commits into from Oct 26, 2021

Conversation

PierreRambaud
Copy link
Contributor

@PierreRambaud PierreRambaud commented Sep 6, 2021 •
edited

Questions Answers
Branch? develop
Description? Remove the upgrade directory, everything has to be done by the autoupgrade module.
Type? improvement
Category? CO
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes #23240
How to test? CI is green.
Possible impacts? Please indicate what parts of the software we need to check to make sure everything is alright.

BC breaks

The class PrestaShopBundle\Install\Upgrade no longer exists

This change is Reviewable

@PierreRambaud PierreRambaud requested a review from a team as a code owner September 6, 2021 09:05
@prestonBot prestonBot added develop Branch Improvement Type: Improvement BC break Type: Introduces a backwards-incompatible break labels Sep 6, 2021
Progi1984
Progi1984 previously approved these changes Sep 6, 2021
View reviewed changes
@Progi1984 Progi1984 added this to the 8.0.0 milestone Sep 6, 2021
matthieu-rolland
matthieu-rolland reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpstan issue, lgtm otherwise

Progi1984
Progi1984 previously approved these changes Sep 7, 2021
View reviewed changes
atomiix
atomiix previously approved these changes Sep 7, 2021
View reviewed changes
@PierreRambaud
Copy link
Contributor Author

I'll move all files to the autoupgrade modules (sync them), and add the migrateSettingsFile if needed

@Progi1984 Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label Sep 7, 2021
@PierreRambaud PierreRambaud added the Must-have Kanban prioritization: issue must be included in next version label Sep 8, 2021
@PierreRambaud PierreRambaud mentioned this pull request Sep 16, 2021
Merged
@khouloudbelguith khouloudbelguith removed the Waiting for QA Status: action required, waiting for test feedback label Sep 28, 2021
sowbiba
sowbiba previously approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@sowbiba sowbiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many deleted files. I can't accept this :trollface:

You have a conflict ;)

@PierreRambaud
Copy link
Contributor Author

Too many deleted files. I can't accept this :trollface:

You have a conflict ;)

Rebased 3 days ago, I'll wait for QA notification before rebasing again, and again

matks
matks previously approved these changes Oct 5, 2021
View reviewed changes
@Progi1984
Copy link
Contributor

@PierreRambaud Could you rebase before sending it in QA ? Thanks

@Progi1984 Progi1984 added the Waiting for author Status: action required, waiting for author feedback label Oct 11, 2021
@PierreRambaud
Copy link
Contributor Author

@PierreRambaud Could you rebase before sending it in QA ? Thanks

As I said above, I'll wait for QA to be available, before rebasing again (already did a lot of time 😅 )

jolelievre
jolelievre previously approved these changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PierreRambaud

Maybe just indicate in the BC break description, that PrestaShop can no longer self-upgrade, especially with the CLI script which is documented here https://devdocs.prestashop.com/1.7/basics/keeping-up-to-date/upgrade/#how-to-upgrade-prestashop

We'll have to indicate this script is no longer available in the doc and invite people to rely on the autoupgrade module (until maybe a more generic and reusable library/repository is proposed)

app/config/set_parameters.php
@@ -27,14 +27,6 @@
use PrestaShopBundle\Install\Upgrade;

$parametersFilepath = __DIR__ . '/parameters.php';
if (!file_exists($parametersFilepath)) {
// let's check first if there's some old config files which could be migrated
if (Upgrade::migrateSettingsFile() === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, how is the parameters.php file generated now? From what I remember it was only created by this method, even if the name doesn't suggest it I believe it was the only code responsible for generating this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file is generate in config/bootstrap.php this one is for old PrestaShop version and should be handle by the auotupgrade module. Not with a dirty check here :p

@PierreRambaud PierreRambaud dismissed stale reviews from jolelievre, matks, and sowbiba via 8f6ae53 October 13, 2021 09:23
@PierreRambaud PierreRambaud removed the Waiting for author Status: action required, waiting for author feedback label Oct 13, 2021
atomiix
atomiix previously approved these changes Oct 13, 2021
View reviewed changes
matks
matks previously approved these changes Oct 14, 2021
View reviewed changes
@PierreRambaud PierreRambaud dismissed stale reviews from matks and atomiix via ebe16a0 October 15, 2021 12:49
@Progi1984 Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label Oct 25, 2021
@PierreRambaud PierreRambaud removed the Waiting for QA Status: action required, waiting for test feedback label Oct 26, 2021
@PierreRambaud PierreRambaud merged commit 11195b8 into PrestaShop:develop Oct 26, 2021
@PierreRambaud PierreRambaud deleted the fix/remove-upgrade-scripts branch October 26, 2021 12:12
@matks matks added the Needs documentation Needs an update of the developer documentation label Jul 19, 2022
@kpodemski kpodemski added Documentation ✔️ Developer documentation is up-to-date and removed Needs documentation Needs an update of the developer documentation labels Jul 28, 2022
@PrestaEdit PrestaEdit mentioned this pull request Aug 10, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Progi1984 Progi1984 Progi1984 approved these changes

@atomiix atomiix atomiix approved these changes

@matthieu-rolland matthieu-rolland Awaiting requested review from matthieu-rolland

@jolelievre jolelievre Awaiting requested review from jolelievre

@sowbiba sowbiba Awaiting requested review from sowbiba

@matks matks Awaiting requested review from matks

Assignees
No one assigned
Labels
BC break Type: Introduces a backwards-incompatible break develop Branch Documentation ✔️ Developer documentation is up-to-date Improvement Type: Improvement Must-have Kanban prioritization: issue must be included in next version
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Move upgrade scripts to the autoupgrade module
10 participants
@PierreRambaud @Progi1984 @matthieu-rolland @atomiix @matks @sowbiba @jolelievre @kpodemski @khouloudbelguith @prestonBot