add occ app:install command by sualko · Pull Request #5644 · nextcloud/server

sualko · 2017-07-07T06:45:16Z

This new command downloads the specific app from the app store, but doesn't enable it.

nickvergessen · 2017-07-07T09:26:00Z

+	 */
+	public function completeArgumentValues($argumentName, CompletionContext $context) {
+		if ($argumentName === 'app-id') {
+			$allApps = \OC_App::getAllApps();


Since the apps you want to install are not available locally, you can also not autocomplete them.

I'd just delete the complete* methods and the implements CompletionAwareInterface

nickvergessen · 2017-07-07T09:26:13Z

But nice otherwise

juliusknorr · 2017-07-07T10:38:33Z

+			$installer->downloadApp($appId);
+			$result = $installer->installApp($appId);
+		} catch(Exception $e) {
+			$output->writeln('Error: ' . $ex->getMessage());


Should be $e (see line above)

codecov · 2017-07-07T10:44:35Z

Codecov Report

❗ No coverage uploaded for pull request base (master@174cd4f). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #5644   +/-   ##
=========================================
  Coverage          ?   54.49%           
  Complexity        ?    21309           
=========================================
  Files             ?     1309           
  Lines             ?    82358           
  Branches          ?     1327           
=========================================
  Hits              ?    44884           
  Misses            ?    37474           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
core/Command/App/Install.php	`0% <0%> (ø)`	`5 <5> (?)`
core/register_command.php	`0% <0%> (ø)`	`0 <0> (?)`

ChristophWurst · 2017-07-07T11:21:02Z

+
+class Install extends Command {
+
+	public function __construct() {


unused and thus can be removed.

ChristophWurst · 2017-07-07T11:23:23Z

+		}
+
+		try {
+			$installer = new Installer(


@nickvergessen do we have DI for commands? I'd prefer injecting an object over building it manually. This approach could fail as soon as the constructor parameters of change. Or maybe we could at least query the class from the DI container and have it constructed automatically.

We have "semi-injection": https://github.com/nextcloud/server/blob/master/core/register_command.php#L63

So you can build it outside. Not sure if you can do \OC::$server->query(Installer::class) there thou.

sualko · 2017-07-07T11:32:34Z

@@ -0,0 +1,77 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016, ownCloud, Inc.


What's about the copyright header? How should it look like?

If the logic is yours add yourself
If it code from oc leave as is

It's more or less from app.php#L363, so I leave it for now.

enoch85 · 2017-07-07T11:53:33Z

Would it be possible to add a switch for enable like --enable?

enoch85 · 2017-07-07T13:17:12Z

Tested and it gives me this:

root@nextcloud:/var/www/nextcloud# sudo -u www-data php occ app:install videocalls
An unhandled exception has been thrown:
Error: Class 'OC\Core\Command\Db\Migrations\StatusCommand' not found in /var/www/nextcloud/core/register_command.php:89
Stack trace:
#0 /var/www/nextcloud/lib/private/Console/Application.php(94): require_once()
#1 /var/www/nextcloud/console.php(91): OC\Console\Application->loadCommands(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#2 /var/www/nextcloud/occ(11): require_once('/var/www/nextcl...')

Used this file: https://raw.githubusercontent.com/nextcloud/server/add-occ-app-install/core/register_command.php

enoch85 · 2017-07-07T13:34:35Z

~~OK, it works now (just patched the register_command.php instead), but it asks for app-id I think it should work with the name as well, not just app-id.~~

Sorry, it works. But still would be nice with --enable

enoch85 · 2017-07-07T13:47:39Z

👍 from me.

enoch85 · 2017-07-07T13:48:25Z

Also, would it be possible to backport to 12?

nickvergessen · 2017-07-10T08:33:54Z

Also, would it be possible to backport to 12?

You know we don't backport features ;)

enoch85 · 2017-07-10T19:06:09Z

You know we don't backport features ;)

Oh right, don't know why I forgot about that.

sualko · 2017-07-11T14:58:47Z

Would it be possible to add a switch for enable like --enable?

I think this would duplicate the code of the app:enable command. Anything else to do, or are we able to merge this?

enoch85 · 2017-07-13T11:12:00Z

@nickvergessen @juliushaertl Does this look OK to you? I tested and it works in current state.

nickvergessen · 2017-07-13T11:15:42Z

I think this would duplicate the code of the app:enable command. Anything else to do, or are we able to merge this?

I think a simple enable flag (yes|no) would be okay, if you want to enable it for a group the second call is fine I'd say...

rullzer

Can probabaly use some polishing later on. But awesome stuff!

enoch85 · 2017-07-21T11:34:12Z

@juliushaertl And what about you? :)

MorrisJobke · 2017-07-21T20:21:16Z

There is only one thing missing: the sign-off. See https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work how to do it. It's basically one line in the commit message of every commit.

sualko · 2017-07-21T20:23:58Z

That's right, I only signed-off the first commit. What's the best practice for such an case? Rebase and sign-off, or add sign-off to every commit?

sualko · 2017-07-25T08:27:59Z

it should work ;)

It doesn't 😢

MorrisJobke · 2017-07-25T08:39:14Z

It doesn't 😢

Because your master is 18 days old 😉 Also update master and then rebase again ;)

MorrisJobke · 2017-07-25T10:17:09Z

+			);
+			$installer->downloadApp($appId);
+			$result = $installer->installApp($appId);
+		} catch(Exception $e) {


Phan CI job says no:

core/Command/App/Install.php:63 PhanUndeclaredClassCatch Catching undeclared class \OC\Core\Command\App\Exception core/Command/App/Install.php:64 PhanUndeclaredClassMethod Call to method getMessage from undeclared class \OC\Core\Command\App\Exception

https://drone.nextcloud.com/nextcloud/server/359/38

I guess adding a leading \ fixes it 😉

Fixed

enoch85 · 2017-07-25T17:00:17Z

@sualko Still failing. :/

sualko · 2017-07-25T18:33:59Z

I know and it gets annoying, because those tests fail with messages like the following:

PHP Warning:  Redis::connect(): connect() failed:

Waiting for Selenium

Parse error: ./build/.phan/plugins/SqlInjectionCheckerPlugin.php:22

creating sqlite db
  [OCP\Files\NotFoundException]

Those are not very helpful and for me it looks like there is a more general issue in the test setup, because if you look at the commit history of the master branch there are only ❌.

enoch85 · 2017-07-25T18:35:10Z

@MorrisJobke Are tests broken?

MorrisJobke · 2017-07-26T07:28:18Z

@MorrisJobke Are tests broken?

We are still struggling with the docker setup. Sorry for this. I try to get this resolved as soon as possible.

nickvergessen · 2017-07-26T11:01:06Z

bash build/autoloaderchecker.sh
git add lib/composer/
git commit
git push

Still needs this, then more tests should pass ;)

sualko · 2017-07-26T11:32:48Z

@nickvergessen I did this and some more tests succeeded, but the next test run failed with a message that I should run the command again. To my surprise the command removed the lines again. My conclusion is, that I will wait for Morris ok to rerun those tests.

MorrisJobke · 2017-07-26T12:34:39Z

As of now it looks like the tests are actually broken. Somehow the class can't be loaded. Could you please run bash build/autoloaderchecker.sh and commit the result (it is needed anyways - another test failure will be fixed by this 😉)

Signed-off-by: Klaus Herberth <klaus@jsxc.org>

sualko · 2017-07-26T12:38:12Z

There are no changes to commit after running bash build/autoloaderchecker.sh. I also rebased with git rebase -i master and tried again.

sualko · 2017-07-26T12:40:30Z

$ bash build/autoloaderchecker.sh
Global composer found: checking for update
You are already using composer version 1.4.2 (stable channel).

Regenerating autoloader
Generating optimized autoload files

Autoloader up to date. Carry on

Signed-off-by: sualko <klaus@jsxc.org>

MorrisJobke · 2017-07-26T15:11:03Z

There are no changes to commit after running bash build/autoloaderchecker.sh. I also rebased with git rebase -i master and tried again.

Okay ... then my diff was out of date, because now the changes to the composer files are shown. Sorry for the hassle then.

And sorry for the CI mess. We are currently recovering from the long backlog.

sualko · 2017-07-27T12:48:48Z

Okay ... then my diff was out of date

No, it was my fail. I don't know why, but the namespace was missing. After adding it, the autoloaderchecker did his job.

MorrisJobke · 2017-07-27T13:36:26Z

Ignore the 3 drone timeouts ... those are due to a bit overload on that system as of now. Should be fixed soon.

enoch85 · 2017-07-27T15:32:00Z

MorrisJobke merged commit c7757b0 into master 2 hours ago

Yaay!

sualko requested a review from ChristophWurst July 7, 2017 08:36

sualko mentioned this pull request Jul 7, 2017

Install app via command line - occ app:install #1599

Closed

MorrisJobke requested review from LukasReschke, nickvergessen and rullzer July 7, 2017 09:17

MorrisJobke added 3. to review Waiting for reviews enhancement labels Jul 7, 2017

MorrisJobke added this to the Nextcloud 13 milestone Jul 7, 2017

nickvergessen requested changes Jul 7, 2017

View reviewed changes

juliusknorr previously requested changes Jul 7, 2017

View reviewed changes

ChristophWurst reviewed Jul 7, 2017

View reviewed changes

sualko commented Jul 7, 2017

View reviewed changes

nickvergessen approved these changes Jul 13, 2017

View reviewed changes

rullzer approved these changes Jul 21, 2017

View reviewed changes

rullzer force-pushed the add-occ-app-install branch from daa6d9d to ebb137b Compare July 21, 2017 11:13

sualko force-pushed the add-occ-app-install branch from 95953da to 9edfa93 Compare July 24, 2017 13:51

MorrisJobke requested changes Jul 25, 2017

View reviewed changes

sualko force-pushed the add-occ-app-install branch 2 times, most recently from db76747 to 2cb7214 Compare July 25, 2017 14:33

add occ app:install command

3e62a25

Signed-off-by: Klaus Herberth <klaus@jsxc.org>

sualko force-pushed the add-occ-app-install branch from 2cb7214 to 3e62a25 Compare July 26, 2017 12:38

sualko added 2 commits July 26, 2017 15:24

add namespace

7720e19

Signed-off-by: sualko <klaus@jsxc.org>

update autoloader

4868e19

Signed-off-by: sualko <klaus@jsxc.org>

MorrisJobke merged commit c7757b0 into master Jul 27, 2017

MorrisJobke deleted the add-occ-app-install branch July 27, 2017 13:36

gnarea mentioned this pull request Aug 8, 2017

Support downloads from the Nextcloud App Store gnarea/nextcloud-12factor#5

Open

exploide mentioned this pull request Oct 29, 2017

Automatic app updates #7002

Closed

patschi mentioned this pull request Oct 5, 2018

Add occ app:remove CLI command #11627

Merged

gmright2 mentioned this pull request Jan 10, 2023

[Snyk] Security upgrade puppeteer from 1.20.0 to 3.0.0 gmright2/server#110

Open


		class Install extends Command {

		public function __construct() {

Uh oh!

Uh oh!

Conversation

sualko commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enoch85 commented Jul 7, 2017

Uh oh!

enoch85 commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enoch85 commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enoch85 commented Jul 7, 2017

Uh oh!

enoch85 commented Jul 7, 2017

Uh oh!

nickvergessen commented Jul 10, 2017

Uh oh!

enoch85 commented Jul 10, 2017

Uh oh!

sualko commented Jul 11, 2017

Uh oh!

enoch85 commented Jul 13, 2017

Uh oh!

nickvergessen commented Jul 13, 2017

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

enoch85 commented Jul 21, 2017

Uh oh!

MorrisJobke commented Jul 21, 2017

Uh oh!

sualko commented Jul 21, 2017

Uh oh!

sualko commented Jul 25, 2017

Uh oh!

MorrisJobke commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enoch85 commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sualko commented Jul 25, 2017

Uh oh!

enoch85 commented Jul 25, 2017

Uh oh!

MorrisJobke commented Jul 26, 2017

Uh oh!

nickvergessen commented Jul 26, 2017

Uh oh!

sualko commented Jul 26, 2017

Uh oh!

MorrisJobke commented Jul 26, 2017

codecov Bot commented Jul 7, 2017 •

edited

Loading

enoch85 commented Jul 7, 2017 •

edited

Loading

enoch85 commented Jul 7, 2017 •

edited

Loading

enoch85 commented Jul 25, 2017 •

edited

Loading