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

Splitting shipped and installed apps #9

Open
skjnldsv opened this issue Dec 13, 2022 · 15 comments
Open

Splitting shipped and installed apps #9

skjnldsv opened this issue Dec 13, 2022 · 15 comments
Labels
discussion Being discussed

Comments

@skjnldsv
Copy link
Member

skjnldsv commented Dec 13, 2022

Following some discussions that I've seen happening in the past and recently too

Proposal

On every setup, dev AND prod, having a read-only and read-write apps folders should be the preferred and official way.

$ ls -la
drwxr-xr-x   47 admin users    4096 26 nov.  15:27 3rdparty
drwxr-xr-x   31 admin users    4096 18 oct.  15:34 apps
drwxr-xr-x    2 admin users    4096 17 août  16:06 apps2
drwxr-xr-x    2 admin users    4096 29 nov.  09:52 config

Reasoning

  • From a dev perspective, it avoid any script from server potential editing folders/files that are not part of the server repository
  • From a production perspective:
    • Cleaner folder structure
    • Makes it possible to install updated of shipped apps (shipped stays in apps, update goes into apps2
    • Anything else?

Questions

  • Show a warning in setup page ?
  • Strict forbid of new setups if no apps config ?
  • Enforce it straight away without the admin configuring anything ?
  • What would be the apps folder name ? apps2 ? custom_apps ?

cc @PVince81 @ChristophWurst @nickvergessen @blizzz @juliushaertl @icewind1991

@PVince81
Copy link
Member

see also: owncloud/core#30889
it can be done also automatically at install time for new setups to encourage this

@ChristophWurst
Copy link
Member

What if we move the shipped apps out and leave the good old apps for external apps?

@max-nextcloud
Copy link

What if we move the shipped apps out and leave the good old apps for external apps?

I don't quite understand where out would be. Moving all the shipped apps into the main folder of server feels cluttered to me.

What about calling shipped apps components and having a components folder?

@skjnldsv
Copy link
Member Author

Whatever the name is, this is what Christoph was suggestiong :)
Use apps as the default writable apps folder, and another one for what is shipped

@mejo-
Copy link
Member

mejo- commented Dec 14, 2022

I think @ChristophWurst's proposal makes a lot of sense. Something like apps-shipped.

One thought regarding updating shipped apps via app store: Nextcloud whould have to always use the newest version of an app locally available, right? So if Text is shipped in apps-shipped as 1.2.3, then gets updated via app store to 1.2.4 in apps and then the shipped version in apps-shipped updates to 1.3.0, we should either remove the older version from apps automatically during the upgrade process, or disregard it when deciding with version to load.

The scenario of updating shipped apps via app store also is a good reason to not invent a new name for shipped apps in my opinion. Both are apps and both can be updated via app store. It's just that some of them are already shipped by default 🤷

@skjnldsv
Copy link
Member Author

disregard it when deciding with version to load.

We already do that.
We take the bigger version number and use it.
We fixed it a few versions ago (22 or 23 iirc) :)

We cannot update/delete folders of shipped apps, it will cause integrity errors unfortunately

@nickvergessen
Copy link
Member

I will ask all the packagers (VM, docker, snap, ...) for feedback on this, so we take into account what they face

@tobiasKaminsky
Copy link
Member

We cannot update/delete folders of shipped apps, it will cause integrity errors unfortunately

What about the other way around? No shipped versions, but those will be installed during installation.
Then every app is "only" an app via app store.

@nickvergessen
Copy link
Member

That breaks offline installs. But as outlined above an app can exist in multiple dirs and the newest version is used

@szaimen
Copy link

szaimen commented Dec 16, 2022

I'd vote for adding a setup warning if not at least two locations are provided. However I would not enforce it during the installation as it may break existing appliances.

@skjnldsv
Copy link
Member Author

installation as it may break existing appliances

Well, existing appliances do not go through the setup process :)
So this should not be an issue

@nickvergessen
Copy link
Member

I'd vote for adding a setup warning if not at least two locations are provided.

That is exactly why we'd move the current shipped apps. That doesn't require any change on existing setups. It will automatically work out of the box, shipped code we can move as we want and doesn't need a writable folder. And the downloaded appstore apps stay where they are

@kyrofa
Copy link
Member

kyrofa commented Dec 17, 2022

Snap packager here. First of all: big fan of the idea. It's how we've shipped Nextcloud from the beginning: leaving the shipped apps read-only in the <webroot>/apps directory, and adding a writable extra-apps folder elsewhere.

What would be the apps folder name ? apps2 ? custom_apps ?

Initially, I agreed with @ChristophWurst's proposal: leave the apps folder as the writable area, and move the apps bundled with the Nextcloud release elsewhere (release-apps, maybe). That said, that kind of change would break existing configs where folks are already using this kind of setup, like the snap. The config we've been shipping for years has this:

// ...
'apps_paths' => array(
        /**
         * These are the default apps shipped with Nextcloud. They are read-only.
         */
        array(
                'path'=> $snap_current.'/htdocs/apps',
                'url' => '/apps',
                'writable' => false,
        ),

        /**
         * This directory is writable, meant for apps installed by the user.
         */
        array(
                'path'=> $snap_data_current.'/nextcloud/extra-apps',
                'url' => '/extra-apps',
                'writable' => true,
        ),
),
// ...

If you move/change the purpose of the apps folder, you will invalidate all those deployed configs. Patching a user's config in an update is a hairy, risky process that I'd prefer to avoid, personally. So I lean more toward a new extra-apps directory. Whatever we do, we need to keep existing deployments in mind.

@nickvergessen
Copy link
Member

We could hardcode the new directory without adding it to config.php

@kyrofa
Copy link
Member

kyrofa commented Dec 17, 2022

For the sake of my comment, let's say we're considering the following:

  1. Move the existing apps directory to, say, released-apps
  2. Create a new, empty apps directory to serve as the writable apps area

It's not entirely clear which one you're suggesting to hard-code (perhaps both), so I'll cover both here.

Speaking only from the snap perspective, I don't immediately see an issue with hard-coding read-only access to (1) onto the apps_paths (i.e. without requiring it to be in the config). We would need to handle possible conflicts there where e.g. a user might try to add released-apps to their apps_path with writable access, but I assume you have considered that and I will hereby consider it out of scope for this comment.

Hard-coding writable access to (2) onto the apps_paths would be problematic for the snap, though, because the entire webroot is completely read-only. There's no way to make that new apps directory writable as it's on a squashfs image. Also, we already have a config for the apps directory saying it's read-only, which brings me back to the conflict that I was hoping to ignore, but is rearing its head again 😛 .

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

No branches or pull requests

9 participants