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

Setup command #14416

Merged
merged 4 commits into from
Feb 23, 2015
Merged

Setup command #14416

merged 4 commits into from
Feb 23, 2015

Conversation

DeepDiver1975
Copy link
Member

@LukasReschke @PVince81 @th3fallen @icewind1991 please review - THX

In case the owncloud instance is not yet setup the number of commands will be pretty short:

$ ./occ list
ownCloud is not installed - only a limited number of commands are available
ownCloud version 8.1 alpha 1

Usage:
 [options] command [arguments]

Options:
 --help (-h)           Display this help message
 --quiet (-q)          Do not output any message
 --verbose (-v|vv|vvv) Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug
 --version (-V)        Display this application version
 --ansi                Force ANSI output
 --no-ansi             Disable ANSI output
 --no-interaction (-n) Do not ask any interactive question

Available commands:
 check                 check dependencies of the server environment
 help                  Displays help for a command
 list                  Lists commands
 status                show some status information
app
 app:check-code        check code to be compliant
l10n
 l10n:createjs         Create javascript translation files for a given app
maintenance
 maintenance:install   install ownCloud

maintenance:install is the command to be used for installing an instance via the console:

$ ./occ help maintenance:install
ownCloud is not installed - only a limited number of commands are available
Usage:
 maintenance:install [--database="..."] [--database-name="..."] [--database-host="..."] [--database-user="..."] [--database-pass="..."] [--database-table-prefix[="..."]] [--admin-user="..."] [--admin-pass="..."] [--data-dir="..."]

Options:
 --database               Supported database type (default: "sqlite")
 --database-name          Name of the database
 --database-host          Hostname of the database (default: "localhost")
 --database-user          User name to connect to the database
 --database-pass          Password of the database user
 --database-table-prefix  Prefix for all tables (default: oc_)
 --admin-user             User name of the admin account (default: "admin")
 --admin-pass             Password of the admin account
 --data-dir               Path to data directory (default: "/home/deepdiver/Development/ownCloud/core-autotest/data")
 --help (-h)              Display this help message
 --quiet (-q)             Do not output any message
 --verbose (-v|vv|vvv)    Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug
 --version (-V)           Display this application version
 --ansi                   Force ANSI output
 --no-ansi                Disable ANSI output
 --no-interaction (-n)    Do not ask any interactive question

@DeepDiver1975 DeepDiver1975 added this to the 8.1-next milestone Feb 21, 2015
@ghost
Copy link

ghost commented Feb 21, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9831/
Test PASSed.

}
} else {
echo "ownCloud or on of the apps require upgrade - only a limited number of commands is available" . PHP_EOL;
Copy link
Member

Choose a reason for hiding this comment

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

on => one

@karlitschek
Copy link
Contributor

nice 👍

@ghost
Copy link

ghost commented Feb 22, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9833/
Test PASSed.

throw new InvalidArgumentException("Database <$db> is not supported.");
}

$dbUser = $input->getOption('database-user');
Copy link

Choose a reason for hiding this comment

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

Build $options array here instead?

@ghost
Copy link

ghost commented Feb 23, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9845/
Test PASSed.

@ghost
Copy link

ghost commented Feb 23, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9864/
Test PASSed.

@DeepDiver1975
Copy link
Member Author

@bantu @Xenopathic any further objections? Otherwise I'd like to merge this - THX

@RobinMcCorkell
Copy link
Member

Just tested with no options: it attempts to create an sqlite database, then just prints 'Array' and fails. The oC instance isn't installed properly.

EDIT: Same thing when trying to create a MySQL instance.

Logs say it is at core/command/maintenance/install.php#49: Array to string conversion

@RobinMcCorkell
Copy link
Member

During the failed MySQL installation, the DB pass isn't written to the config, which probably explains part of the errors.

@DeepDiver1975
Copy link
Member Author

THX - well - this is because of this bloody errors array being returned and holding different data types ..... 😠

@DeepDiver1975
Copy link
Member Author

@Xenopathic you might want to try it again - thanks for testing!

@ghost
Copy link

ghost commented Feb 23, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9869/
Test PASSed.

@RobinMcCorkell
Copy link
Member

Well I tried my best to break it... but couldn't. Looks like it's a nice robust solution! Only issue I can see is a minor one: the config.php is generated with a NULL trusted_domain, and a bad overwrite.cli.url field: http:///.. I don't know what the implications of this are.

Otherwise, 👍

@LukasReschke
Copy link
Member

NULL trusted_domain

That's erm not really good - we need to have at least one trusted domain or it won't work in real life deployments. (localhost is always whitelisted)

@DeepDiver1975
Copy link
Member Author

That's erm not really good - we need to have at least one trusted domain or it won't work in real life deployments. (localhost is always whitelisted)

I'll have a look ....

@DeepDiver1975
Copy link
Member Author

@LukasReschke @Xenopathic please review and test again - thanks a lot!

@scrutinizer-notifier
Copy link

The inspection completed: 11 new issues, 6 updated code elements

@ghost
Copy link

ghost commented Feb 23, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9874/
Test PASSed.

@LukasReschke
Copy link
Member

Works if executed as webserver user - nice! 👍

Once #14357 is merged the .htaccess will also get generated properly.

@RobinMcCorkell
Copy link
Member

This time I see localhost for both trusted_domains and overwrite.cli.url. While there is no other way around trusted_domains that I can see (once installed, the admin can add a trusted domain easily), perhaps the overwrite.cli.url parameter should be left empty?

@LukasReschke
Copy link
Member

This time I see localhost for both trusted_domains and overwrite.cli.url. While there is no other way around trusted_domains that I can see (once installed, the admin can add a trusted domain easily), perhaps the overwrite.cli.url parameter should be left empty?

That will lead to quite some broken stuff. IMHO it's okay to have this defaulting to localhost as people using occ should be anyways able to configure their system properly afterwards 🙈

@RobinMcCorkell
Copy link
Member

Right, in that case, 👍 🚀 :shipit:

@DeepDiver1975
Copy link
Member Author

Thanks for testing and review, gentlemen!

groot

DeepDiver1975 added a commit that referenced this pull request Feb 23, 2015
@DeepDiver1975 DeepDiver1975 merged commit e87ada8 into master Feb 23, 2015
@DeepDiver1975 DeepDiver1975 deleted the setup-command branch February 23, 2015 21:41
@PVince81
Copy link
Contributor

This broke master due to "OC_Config" having been omitted.

Did Jenkins lie about this PR passing ?

@LukasReschke
Copy link
Member

Did Jenkins lie about this PR passing ?

No. The namespaceOC_Config was added later… The PR passed fine in the first test run.

@PVince81
Copy link
Contributor

Fix here: #14936

@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants