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

Move files_external backend + config logic to core #18160

Closed
PVince81 opened this Issue Aug 10, 2015 · 19 comments

Comments

Projects
None yet
5 participants
@PVince81
Member

PVince81 commented Aug 10, 2015

@DeepDiver1975 @Xenopathic @karlitschek @jmaciasportela

Currently external apps that provide storage backends would require files_external to be enabled too.
Goal is to move that part to core so that the files_external app disappears completely.
The other backends currently provided by files_external should be moved to one or several separate repos eventually.

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Aug 10, 2015

Member

I hate it to move more stuff into the core and make everything less modular. But this might be a case where is actually make sense.

Member

karlitschek commented Aug 10, 2015

I hate it to move more stuff into the core and make everything less modular. But this might be a case where is actually make sense.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Aug 10, 2015

Member

I see no other possibility if we want to support external storage implementations to live isolated on their own apps.

Member

DeepDiver1975 commented Aug 10, 2015

I see no other possibility if we want to support external storage implementations to live isolated on their own apps.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Aug 10, 2015

Member

To be clear: no external storage should be moved to core

Member

DeepDiver1975 commented Aug 10, 2015

To be clear: no external storage should be moved to core

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Aug 10, 2015

Member

In addition, the configuration UI might want to stay in files_external? So only the backend stuff will go to core

Member

Xenopathic commented Aug 10, 2015

In addition, the configuration UI might want to stay in files_external? So only the backend stuff will go to core

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Aug 10, 2015

Member

@Xenopathic which means that if a mount.json exists it would be read but there would be no UI to configure it then ?

Member

PVince81 commented Aug 10, 2015

@Xenopathic which means that if a mount.json exists it would be read but there would be no UI to configure it then ?

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Aug 10, 2015

Member

@PVince81 Yes, in that circumstance that is what would happen. But in the majority of cases, mount.json would not get created, since the UI would not be present. It was an idea mentioned by @DeepDiver1975 some time ago...

Member

Xenopathic commented Aug 10, 2015

@PVince81 Yes, in that circumstance that is what would happen. But in the majority of cases, mount.json would not get created, since the UI would not be present. It was an idea mentioned by @DeepDiver1975 some time ago...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Aug 24, 2015

Member

We might also want to add some public APIs to make it possible for apps to retrieve the list of external storage backends and also instantiate then. See pbek/ownbackup#3 (comment)

Member

PVince81 commented Aug 24, 2015

We might also want to add some public APIs to make it possible for apps to retrieve the list of external storage backends and also instantiate then. See pbek/ownbackup#3 (comment)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 11, 2016

Member

I'm setting this to 9.1 to look into that.

There are some code paths in core that currently check if the app is enabled which is not nice, and bugs like #21784

Member

PVince81 commented Feb 11, 2016

I'm setting this to 9.1 to look into that.

There are some code paths in core that currently check if the app is enabled which is not nice, and bugs like #21784

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 4, 2016

Member

@Xenopathic do you have time for some fun refactoring ? 😄

Member

PVince81 commented Jul 4, 2016

@Xenopathic do you have time for some fun refactoring ? 😄

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 8, 2016

Member

Hmmm, now I see that some apps might be using the \OCA\Files_External namespace already to access some of the APIs. So if we move stuff to core we'd need to add placeholders in the app itself to avoid breaking third party external storage apps during the deprecation period.

Member

PVince81 commented Jul 8, 2016

Hmmm, now I see that some apps might be using the \OCA\Files_External namespace already to access some of the APIs. So if we move stuff to core we'd need to add placeholders in the app itself to avoid breaking third party external storage apps during the deprecation period.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 8, 2016

Member

CC @jvillafanez in case you want to give some input for the general idea of moving files_external service/backend/config classes to core.

Seems we'll also have to decide which are public and which are private.

So far my idea is to have a new namespace \OC\Files\External\Service in core for the services.

Member

PVince81 commented Jul 8, 2016

CC @jvillafanez in case you want to give some input for the general idea of moving files_external service/backend/config classes to core.

Seems we'll also have to decide which are public and which are private.

So far my idea is to have a new namespace \OC\Files\External\Service in core for the services.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 8, 2016

Member

PR here #25422 (WIP)

Member

PVince81 commented Jul 8, 2016

PR here #25422 (WIP)

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Jul 11, 2016

Member

Ideally, the files_external app should be part of the files app, and this one should be isolated. However, taking into account the amount of responsabilities absorbed by core regarding the files app, it make sense that some of the responsabilities from the files_external app are also absorbed.

What should we move to core? Being very general: all the interfaces, but none of the implementations.

  • Interfaces to connect to the backends are already in core, so no need to change anything there. We might consider to separate each implementation into different apps, but not in the near future anyway. The implementation of each backend should be kept outside of core.
  • Interfaces to configure the backends. The implementation of common components (textfields, checkboxes, etc) might also be moved to core, BUT custom components should be able to be created per each backend.
  • Interfaces to provide authentication mechanisms. The same as the previous point.
  • Additional services available (such as scanner, watcher, cache, etc). The default implementation might be in core, but custom implementations might be provided by others. (I think this needs some kind of rework because those services are very coupled with a DB implementation, and they're making several assumptions which depends on the specific implementation).
  • General CLI actions over the storages. This includes configuration, listing, scanning, etc.

There are other things to consider:

  • We should consider actions over the storage configuration and / or authentication mechanism (for example, password reset).
  • We should also consider communication between storages (to be decided if the communication will be among the storages of the same type, or with any storage)
Member

jvillafanez commented Jul 11, 2016

Ideally, the files_external app should be part of the files app, and this one should be isolated. However, taking into account the amount of responsabilities absorbed by core regarding the files app, it make sense that some of the responsabilities from the files_external app are also absorbed.

What should we move to core? Being very general: all the interfaces, but none of the implementations.

  • Interfaces to connect to the backends are already in core, so no need to change anything there. We might consider to separate each implementation into different apps, but not in the near future anyway. The implementation of each backend should be kept outside of core.
  • Interfaces to configure the backends. The implementation of common components (textfields, checkboxes, etc) might also be moved to core, BUT custom components should be able to be created per each backend.
  • Interfaces to provide authentication mechanisms. The same as the previous point.
  • Additional services available (such as scanner, watcher, cache, etc). The default implementation might be in core, but custom implementations might be provided by others. (I think this needs some kind of rework because those services are very coupled with a DB implementation, and they're making several assumptions which depends on the specific implementation).
  • General CLI actions over the storages. This includes configuration, listing, scanning, etc.

There are other things to consider:

  • We should consider actions over the storage configuration and / or authentication mechanism (for example, password reset).
  • We should also consider communication between storages (to be decided if the communication will be among the storages of the same type, or with any storage)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 11, 2016

Member

@jvillafanez I'd say for now the goal is only to move all the code that is not storage-specific from files_external to core. And only leave the storage backend implementations in "files_external", which might also be moved to separate apps in the future.

As for the "GlobalStorageServices" and "UserGlobalStorageService" these implementations should also be in code, considering that they are exposed through the server container already. This might also imply moving the CLI commands from files_external to core as well to manage configurations.

The end result will be that "files_external" really becomes "here's a bunch of storage backends you might want to use" but is optional, and the external storage stuff and its config can then work as part of core.

Member

PVince81 commented Jul 11, 2016

@jvillafanez I'd say for now the goal is only to move all the code that is not storage-specific from files_external to core. And only leave the storage backend implementations in "files_external", which might also be moved to separate apps in the future.

As for the "GlobalStorageServices" and "UserGlobalStorageService" these implementations should also be in code, considering that they are exposed through the server container already. This might also imply moving the CLI commands from files_external to core as well to manage configurations.

The end result will be that "files_external" really becomes "here's a bunch of storage backends you might want to use" but is optional, and the external storage stuff and its config can then work as part of core.

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Jul 11, 2016

Member

@PVince81 I completely agree, but I think it's a good time to look ahead and make plans for what's to come.

As for the "GlobalStorageServices" and "UserGlobalStorageService" these implementations should also be in code, considering that they are exposed through the server container already. This might also imply moving the CLI commands from files_external to core as well to manage configurations.

Yep. That part isn't storage-specific and as such, it should be in core. Those services (and the UserStorageService) access to already-configured mount points, and that responsability should fall under core.

We need to make sure that, even though the backends could provide custom configuration elements (a combobox, for example) that aren't defined in core, those elements are also acknowledged by core and can be configured using the "standard" CLI commands to configure the external storages. I think this is an important point: the core can provide standard components in order to use them easily, but it should be able to acknowledge other custom components and interact with them

Member

jvillafanez commented Jul 11, 2016

@PVince81 I completely agree, but I think it's a good time to look ahead and make plans for what's to come.

As for the "GlobalStorageServices" and "UserGlobalStorageService" these implementations should also be in code, considering that they are exposed through the server container already. This might also imply moving the CLI commands from files_external to core as well to manage configurations.

Yep. That part isn't storage-specific and as such, it should be in core. Those services (and the UserStorageService) access to already-configured mount points, and that responsability should fall under core.

We need to make sure that, even though the backends could provide custom configuration elements (a combobox, for example) that aren't defined in core, those elements are also acknowledged by core and can be configured using the "standard" CLI commands to configure the external storages. I think this is an important point: the core can provide standard components in order to use them easily, but it should be able to acknowledge other custom components and interact with them

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 11, 2016

Member

I think custom controls already work. When declaring a storage backend, you can specify a custom JS file that will manipulate the UI elements to replace some fields with custom elements (ex: date picker).

The CLI works in a generic manner so it would accept arbitrary values and should already work with custom fields.

Member

PVince81 commented Jul 11, 2016

I think custom controls already work. When declaring a storage backend, you can specify a custom JS file that will manipulate the UI elements to replace some fields with custom elements (ex: date picker).

The CLI works in a generic manner so it would accept arbitrary values and should already work with custom fields.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 8, 2016

Member

The backend and API stuff was already moved to core, so for all storages that use the core API the files_external app does not need to be enabled for the storages to work.

However the UI and OCC commands still needs to be ported to core for this to be more complete.

Member

PVince81 commented Dec 8, 2016

The backend and API stuff was already moved to core, so for all storages that use the core API the files_external app does not need to be enabled for the storages to work.

However the UI and OCC commands still needs to be ported to core for this to be more complete.

@PVince81 PVince81 self-assigned this Dec 12, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 27, 2017

Member

Will move the web UI and OCC commands later

Member

PVince81 commented Jan 27, 2017

Will move the web UI and OCC commands later

@PVince81 PVince81 modified the milestones: 10.1, 10.0 Jan 27, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 30, 2017

Member

There isn't enough time left to move GUI and OCC commands for now.
To avoid breaking the user experience, the files_external app will now be always enabled by default: #27051

This until we can finish these follow up tasks:

  • move storage backends to separate apps: #27049
  • move GUI / OCC commands to core: #27050
Member

PVince81 commented Jan 30, 2017

There isn't enough time left to move GUI and OCC commands for now.
To avoid breaking the user experience, the files_external app will now be always enabled by default: #27051

This until we can finish these follow up tasks:

  • move storage backends to separate apps: #27049
  • move GUI / OCC commands to core: #27050

@PVince81 PVince81 closed this Jan 30, 2017

@DeepDiver1975 DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment