-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add command to get and set config for external mounts #20710
Conversation
Woah, nice! (Will review in the next few days) |
@icewind1991 can you always post a link to the original ticket it is related to ? For #20368 |
Looks good 👍 |
->setName('files_external:config') | ||
->setDescription('Manage backend configuration for a mount') | ||
->addArgument( | ||
'action', |
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.
It would be cleaner to get if there is no value passed, and set if there is a value passed, removing the need for a separate command line argument.
Proposed example usage:
./occ files_external:config 42 host #prints 'localhost'
./occ files_external:config 42 host abc.example.com #sets host
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.
Seems cleaner yes
219ac67
to
6ed4dcc
Compare
Removed the get/set argument, action is now determined based on if the value is defined |
Also added command to set mount options |
$key = $input->getArgument('key'); | ||
$mount = $this->globalService->getStorage($mountId); | ||
|
||
if (!$mount instanceof StorageConfig) { |
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 storage service will throw an exception itself if the storage cannot be found, so this block is unneeded (but it might be advisable to wrap the getStorage()
in a try catch to print a better message)
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.
Fixed
e7efbe9
to
1e1cb7a
Compare
$mount = $this->globalService->getStorage($mountId); | ||
} catch (NotFoundException $e) { | ||
$output->writeln('<error>Mount with id "' . $mountId . ' not found, check "occ files_external:list" to get available mounts"</error>'); | ||
return; |
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.
Return with a non-zero code so shell scripts can detect the error
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.
@icewind1991 can you fix this and then I'll count @Xenopathic's LGTM as 👍
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.
done
LGTM so far, just one question: how can we set non-string values, e.g. integers or booleans? |
You can just enter them directory, bools, ints and floats are all handled correctly |
@Xenopathic counts as 👍 ? |
@Xenopathic @DeepDiver1975 merge? |
1e1cb7a
to
1347e33
Compare
👍 |
Add command to get and set config for external mounts
Usage:
Currently this only works for admin configured storage due to limitations in the way we store mounts, with #20370 it can be extended to properly handle user configured mounts
cc @PVince81 @Xenopathic