-
Notifications
You must be signed in to change notification settings - Fork 183
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
Expose configuration for the reva archiver #2509
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
@jvillafanez this should also need some proxy configuration, so that oC Web can use this endpoint. Probably we also should add an capability @kulmann ? |
💥 Acceptance tests Web-Tests-ocis-ocis-storage-1 failed. The build is cancelled... |
there is already a capability, see https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/cloud/capabilities/capabilities.go#L105 and https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/data/capabilities.go#L110 but it seems we are not setting it https://github.com/owncloud/ocis/blob/master/storage/pkg/command/frontend.go#L66-L72 @jvillafanez you could try setting it in the filesCfg. |
Wrong context? The links point to "favorites", and I don't see any reference to the archiver there. The PR in reva implementing the archiver doesn't have any change around capabilities neither. |
Default route to the archiver added to the proxy.
|
urgh yes ... sorry I was confused. I think a new files capability
|
@@ -147,6 +147,13 @@ func frontendConfigFromStruct(c *cli.Context, cfg *config.Config, filesCfg map[s | |||
"timeout": 86400, | |||
"insecure": true, | |||
}, | |||
"archiver": map[string]interface{}{ | |||
"prefix": cfg.Reva.Frontend.ArchiverPrefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is a path, then it is on the same host as the capabilities. But it could be on a dedicated host, eg https://archiver.owncloud.com
.
"prefix": cfg.Reva.Frontend.ArchiverPrefix, | |
"url": cfg.Reva.Frontend.ArchiverURL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already a capability, see https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/cloud/capabilities/capabilities.go#L105 and https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/data/capabilities.go#L110
Wrong context? The links point to "favorites", and I don't see any reference to the archiver there. The PR in reva implementing the archiver doesn't have any change around capabilities neither.
urgh yes ... sorry I was confused. I think a new files capability
archives
makes sense. But I would strongly prefer a url endpoint value that can be used to tell clients chere to go without having to hardcode it in the clients. For example+ "archiver": map[string]interface{}{ + "endpoint": "https://localhost:9200/archiver", + "max_num_files": 5000, + "max_size": 1000000, + },
Probably the endpoint should be a relative path only? We get the backend URL from a config file already (at least in web client). This way you'd go for something like archiverUrl = config.server + capabilities.files.archiver.endpoint
in the frontend? @kulmann
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it should be possible for the rest of the services (datagateway, appprovider, etc) to do the same... so I think we should prioritize consistency in this case.
We could add a comment to rise awareness or adjust the usage string in the command line, although we might need to adjust the strings of the rest of the services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No preference from my side. Whatever is easier (not only for implementation, but also for admin who needs to configure it). In the web ui we have both situations already - e.g. the downloadURL comes as absolute URL form the backend while we do most API requests via relative URLS with the server URL from the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the capabilities, like the /.well-known/openid-configuration
endpoint should allow clients to configure themselves. I agree with @jvillafanez regarding consistency. All endpoints should be URLs. That includes relative URLs like just a path: /archiver
as well as https://otherhost.test/archiver
.
This allows the server to configure clients to use custom implementations of services. All clients should be able to handle absolute and relative URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix is just a "namespace" for the handlers. There is no full URL in that case...
In the capabilities, you will be able to add a full URL, see #2529
capabilities will be tackled in cs3org/reva#2088 |
93ac175
to
889b30c
Compare
@@ -379,6 +383,10 @@ func defaultPolicies() []config.Policy { | |||
Endpoint: "/signin/", | |||
Backend: "http://localhost:9130", | |||
}, | |||
{ | |||
Endpoint: "/archiver", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvillafanez I made the proxy route also match /archiver
so that we can use curl -u einstein:relativity -k 'https://localhost:9200/archiver?dir=/home/Foo' --output Foo.tar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok for me, although I don't know if we have plans to add additional actions against the archiver service at some point.
For example, if we want to archive huge folder, we might want to provide async actions such as /archiver/async?dir=folder
to create the request, /archiver/async/{id}/state
to check the state (queued, archiving fileA, ready...), /archiver/async/{id}
to download the archive if finished.
This allows more flexibility since we wouldn't need to touch the routing.
Kudos, SonarCloud Quality Gate passed! |
@@ -147,6 +147,13 @@ func frontendConfigFromStruct(c *cli.Context, cfg *config.Config, filesCfg map[s | |||
"timeout": 86400, | |||
"insecure": true, | |||
}, | |||
"archiver": map[string]interface{}{ | |||
"prefix": cfg.Reva.Frontend.ArchiverPrefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix is just a "namespace" for the handlers. There is no full URL in that case...
In the capabilities, you will be able to add a full URL, see #2529
Change the full URL is possible via Capabilities, not in the http handler in the archiver service
Note for future readers. The API endpoint in The API endpoint here in oCIS is |
Description
Expose the reva archiver configuration
Related Issue
#2507
Motivation and Context
How Has This Been Tested?
ocis server
The tar file is downloaded and contains the files that were present in the folder.
Screenshots (if appropriate):
Types of changes
Checklist: