fix(ocm): add data_server_url to ocm storage provider #10440
Conversation
|
| StorageRoot string `yaml:"storage_root" env:"OCM_OCM_STORAGE_PROVIDER_STORAGE_ROOT" desc:"Directory where the ocm storage provider persists its data like tus upload info files." introductionVersion:"5.0"` | ||
| Insecure bool `yaml:"insecure" env:"OCM_OCM_STORAGE_PROVIDER_INSECURE" desc:"Disable TLS certificate validation for the OCM connections. Do not set this in production environments." introductionVersion:"5.0"` | ||
| StorageRoot string `yaml:"storage_root" env:"OCM_OCM_STORAGE_PROVIDER_STORAGE_ROOT" desc:"Directory where the ocm storage provider persists its data like tus upload info files." introductionVersion:"5.0"` | ||
| DataServerURL string `yaml:"data_server_url" env:"OCM_OCM_STORAGE_DATA_SERVER_URL" desc:"URL of the data server, needs to be reachable by the data gateway provided by the frontend service or the user if directly exposed." introductionVersion:"7.0.0"` |
There was a problem hiding this comment.
This new envvar is very hard to understand what it is good for. Please add a text in the readme to explain in more detail.
There was a problem hiding this comment.
The text is a copy from https://github.com/owncloud/ocis/blob/master/services/storage-users/pkg/config/config.go#L29 😆 I have a hard time coming up with something better that is brief enough to fit into a comment here. I'll try.
There was a problem hiding this comment.
I must agree to @rhafer that it's a very precise and understandable description, if you know what:
- a data gateway is
- a data server is
But this is oCIS architecture fundamentals that can not be covered in a config option description
There was a problem hiding this comment.
I proposed to do a brief explanation in the readme to help admins to understand. A dev most likely will be fine with the envvar description only.
There was a problem hiding this comment.
I proposed to do a brief explanation in the readme to help admins to understand
I agree that a better description of the architecture is needed here. (I am not even sure I understand it well enough to properly describe it).
But this PR is fixing a release blocker. So I think we should merge it independently of the poor documentation. That issue is broader anyway it affects every storageprovider.
Fixes #10358