Feedback

[julien-nc]

Hey hey, first of all, it was very nice to read your code. It's a huge app and it's already pretty clean :+1: :blue_heart:

Here are some random remarks and questions:

• You can use static functions as callback in array_map and array_filter as long as you don't use $this in the callback

  array_map(static function (IUser $user) {
  return $user->getUID();
  }, $this->userManager->searchDisplayName(''));

• Maybe you can rename the command classes to more explicit names like Enable to EnableApp or EnableExApp so it's more obvious it does not mean "enable the external app system".
• Would it make sense to use named constants for scope groups? They could be declared as public const in the ExAppApiScopeService class.
• Any reason to set Php min version to 8.1? NC 26 supports Php 8.0.
• As far as I understand, it's possible to set the config value "sensitive" flag each time you set a value, right? Could we do something like making this command parameter optional? And if it's not set:
  • set "sensitive" to false if the config key does not exist
  • keep the "sensitive" value if the config key already exists
• We can talk about that later but I'm interested in the daemon concept. It's the entity to manage apps on a different host than the NC server, right? Is the local instance considered as a daemon or is it handle very differently?
• As far as I understand, exApps are not declaring anything statically, right? They have to make multiple OCS API calls at the right moment. Maybe this could be factorized in one "initialization" request in which we could group things. It would be a sort of equivalent to sending an info.xml. Maybe it does not make sense. You tell me.
• I see that IControllerMethodReflector is deprecated (in AppEcosystemAuthMiddleware). Is there any obstacle to use Php8 method attributes? Not a big deal but it might save us maintenance/time to directly use Php8 stuff.
• Really not a big deal but

  new Vue({
  el: '#app_ecosystem_v2_settings',
  render: h => h(AdminSettings),
  })

  can be replaced with

  const View = Vue.extend(AdminSettings)
  new View().$mount('#app_ecosystem_v2_settings')

  Which I think will be easier to migrate to Vue 3

• Why do you expose the loglevel in the capabilities?

Looking forward to continue the discussion in a call.

[bigcat88]

• Any reason to set Php min version to 8.1? NC 26 supports Php 8.0.

$hashContext = hash_init('xxh64'); available only from PHP 8.1

As far as I understand, it's possible to set the config value "sensitive" flag each time you set a value, right? Could we do something like making this command parameter optional? And if it's not set:

• set "sensitive" to false if the config key does not exist
• keep the "sensitive" value if the config key already exists

seems right, we'll do this

• Why do you expose the loglevel in the capabilities?

ExApp need to know it, to not spam network with requests. It is a private capability, not public.

• We can talk about that later but I'm interested in the daemon concept. It's the entity to manage apps on a different host than the NC server, right? Is the local instance considered as a daemon or is it handle very differently?

Docker daemon works the same for remote deploy as for local Difference is only in the detecting URL of the App. We will try today to make graphics docs with pictures today for this, it is in our roadmap for today/tomorrow.

We decided to make deploy method pluggable, so it can be expandable with Kubernetes in future, or usual Python Virtual environment for example, if someone will be interested in implementing such methods.

[andrey18106]

• Maybe you can rename the command classes to more explicit names like Enable to EnableApp or EnableExApp so it's more obvious it does not mean "enable the external app system".

I have named ExApp commands as it was done in Core apps commands, that's why they grouped by folders (namespaces) and I think it's pretty readable.

Would it make sense to use named constants for scope groups? They could be declared as public const in the ExAppApiScopeService class.

I have been thinking about generic way of naming of scopes, but I'm not sure it will be too convenient to support its changes also in constants. It's more complicated if we allow to register scopes outside by others. But if it's OK to make constants just for our API Scopes then it makes sense. For now, we do not considering constants usage, it's not required in our code.

As far as I understand, it's possible to set the config value "sensitive" flag each time you set a value, right?

Yes, good catch, will adjust it.

We can talk about that later but I'm interested in the daemon concept. It's the entity to manage apps on a different host than the NC server, right? Is the local instance considered as a daemon or is it handle very differently?

As Alexander already answered, daemon (containers orchestrator) can be local or remote. And we want to make it extensible, so we could register another daemon types in a future (factorize it), e.g. Kubernetes.

As far as I understand, exApps are not declaring anything statically, right? They have to make multiple OCS API calls at the right moment. Maybe this could be factorized in one "initialization" request in which we could group things. It would be a sort of equivalent to sending an info.xml. Maybe it does not make sense. You tell me.

It's one of the flows to discuss. For now it's not declared statically, ExApp registers it manually in their enable/disable process.

I see that IControllerMethodReflector is deprecated (in AppEcosystemAuthMiddleware). Is there any obstacle to use Php8 method attributes? Not a big deal but it might save us maintenance/time to directly use Php8 stuff.

Yes you are right, we can remove it, I have used AppEcosystemAuth for the first time in annotations.

Vue frontend stuff it's just boilerplate for now, we do not touch it yet. I probably will need some help with it later.

Thank you for feedback!

[bigcat88]

As far as I understand, it's possible to set the config value "sensitive" flag each time you set a value, right?

Yes, good catch, will adjust it.

We should add optional sensitive option to setAppConfigValue controller, so the App can set this flag too.

[bigcat88]

I re-read the feedback again and it seems like everything that has been written here has already been done, so I’m closing it..