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

Added DDEV command to install OM (incl. sample data) #3248

Merged
merged 14 commits into from
Jul 3, 2023
Merged

Added DDEV command to install OM (incl. sample data) #3248

merged 14 commits into from
Jul 3, 2023

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented May 12, 2023

Description (*)

Adds ddev-command ddev install-openmage to install OM w/ or w/o sample data.

There a three optional flags ...

-d use default values for admin-user, db, ... w/o getting asked for
-s install sample data ...
-k keep sample data package (to avoid to download it again and again)
-q drop current install and re-install with default values

Run ddev install-openmage -d -s to install OM /w sample data.

Questions or comments

Install works only WITH sample data!

FAILED
ERROR: Error in file: "/var/www/html/app/code/core/Mage/Customer/sql/customer_setup/mysql4-data-upgrade-1.4.0.0.7-1.4.0.0.8.php" - Notice: Undefined offset: 0  in /var/www/html/app/code/core/Mage/Core/Model/App.php on line 1005

Updates welcome.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the ddev label May 12, 2023
@addison74
Copy link
Contributor

I will test this PR these days, being a fan of DDEV on Windows + Docker + WSL2.

BTW, we have not yet determined what happens with the local.xml file which is automatically created by DDEV when we configure a new project and thus we cannot access to the Backend running the installation. Maybe we should also make an command to run a query for creating an admin account with a simple password. We had this conversion here #2648.

@addison74
Copy link
Contributor

@sreichel - I see that you addressed in the script the issue of creating the administrator account for Backend.

@sreichel
Copy link
Contributor Author

@addison74 in case of an existing local.xml the script stops. It just to (re-)install OM with default ddev values.

... drops your db w/o asking and install openmage w/ sample data
@addison74
Copy link
Contributor

A few comments

1 - If the command ddev install-openmage is run and the question "OpenMage is already installed. Delete local.xml and drop database[y/N]:" is answered with no, the error 'Failed to run install-openmage : exit' is displayed status 1'. However it is not a failure of running the script, but it is coming from the install.php file. We cannot do more about it.

2 - DDEV provides support for SSL certificates. The arguments in the install script use_secure and use_secure_admin should be set to 'yes'.

3 - You can work a little on the phrases

"OpenMage is already installed. Delete local.xml and drop the database? [y/N] :". We keep the same format like in "Install sample data? [y/N]", it is a question.

"Install sample data? [y/N] :". You missed the colon.

Otherwise the script has no other issues and it is extremely useful. I recommend everyone who tests OpenMage to use (DDEV + Docker + WSL2), especially in Windows, if you do not work directly in a Linux distribution.

.ddev/commands/web/install-openmage Outdated Show resolved Hide resolved
.ddev/commands/web/install-openmage Outdated Show resolved Hide resolved
@addison74
Copy link
Contributor

In all read lines we need a blank space at the end.

Before: read -r -p "Install sample data? [y/N]"
After: read -r -p "Install sample data? [y/N] "

In this way the input from the keyboard will be more visible.

@addison74
Copy link
Contributor

The name of the command should start with openmage- prefix, so that we can add other commands related to OpenMage,that are easy to remember. e.g,

openmage-install = what you just created in this PR
openmage-admin = update/create admin account (will come)
...

@OpenMage OpenMage deleted a comment from sreichel May 14, 2023
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
@addison74
Copy link
Contributor

The functionality is exactly what it needs. I made only a few proposals to improve the relationship with the user. Thank you @sreichel for this PR that improves OpenMage's testing mode and helps by reducing the time it takes to install a new instance.

Copy link
Contributor

@addison74 addison74 left a comment

Choose a reason for hiding this comment

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

Only 4 modifications I request :)

.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
.ddev/commands/web/openmage-install Outdated Show resolved Hide resolved
@sreichel sreichel added the hold label May 14, 2023
addison74
addison74 previously approved these changes May 14, 2023
@sreichel sreichel removed the hold label May 14, 2023
@sreichel sreichel marked this pull request as draft May 14, 2023 16:13
@sreichel sreichel marked this pull request as ready for review May 14, 2023 16:19
@sreichel
Copy link
Contributor Author

@addison74 can you please test without sample data?

@addison74
Copy link
Contributor

PHP 7.4.33 - I was able to install both variants. There were no errors when I used the variant without installing the sample data.
PHP 8.1.16 - The same here. SUCCESS.
PHP 8.2.3 - The same here too. SUCCESS.

@sreichel
Copy link
Contributor Author

Strange. Does table-prefix work for you?

@addison74
Copy link
Contributor

I was able to add a prefix to the tables, see the images below. PHP 8.2.3:

table_prefix

table_prefix-2

@sreichel
Copy link
Contributor Author

When it works for you a default prefix should be added. (om_).

Will add some options later ... like session storage ...

@sreichel
Copy link
Contributor Author

@addison74 why did you delete my comment about n98-magerun?

Most commands work ... just needs to get tested explicitly.

@addison74
Copy link
Contributor

My request was off-topic to this PR and I deleted it, there were two messages. In the meantime I opened a PR that solves that situation differently from n98-magerun, where it is a lot of work to be brought to compatibility with PHP8.1. I deleted your answer because the question no longer exists.

@sreichel sreichel marked this pull request as draft May 14, 2023 23:19
@sreichel sreichel marked this pull request as ready for review May 15, 2023 14:41
Changes in phrases and where the /media and /skin directories are copied
Copy link
Contributor

@addison74 addison74 left a comment

Choose a reason for hiding this comment

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

Please approve this PR. I have been using it for some time and there are no issues. I made some changes in the displayed phrases and corrected the directory where they are copied from the downloaded archive /media and /skin directories.

@fballiano
Copy link
Contributor

@addison74 what about your comment about table prefixes?

@addison74
Copy link
Contributor

The command deals with two situations:

  1. installation without Sample Data and here the table prefix must be used.

  2. installation with Sample Data, here you can no longer use the prefix because the tables are already created.

I agree with Sven in the first case to cover the situation of the table prefix from the very beginning. I started from the idea that what is being worked on in DDEV can be put into production and that's why I didn't want a prefix. But it can be easily solved by exporting the database and executing a sed command to delete the prefix. I haven't analyzed it, but I think that a DDEV command can be created to do this automatically.

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

approving because asked, not tested

@fballiano fballiano merged commit 748b79d into OpenMage:main Jul 3, 2023
@sreichel sreichel deleted the ddev-reinstall branch July 11, 2023 00:10
@sreichel
Copy link
Contributor Author

@fballiano you wrote you are not using ddev ... its just a shell script that should work w/o ddev too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants