-
Notifications
You must be signed in to change notification settings - Fork 22
Add opencast.external_api_node
config value
#1221
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 opencast.external_api_node
config value
#1221
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This allows admins to override which OC node is used for external API operations. Previously, `sync_node` was used, which was not always correct. Fixes elan-ev#1216
23e63b2
to
4124484
Compare
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.
Code looks reasonable. Only very minor nitpicks which one could discuss.
I did not test this on a distributed system, though!
@@ -30,6 +30,7 @@ pub(crate) async fn run(shared: &args::Shared, args: &Args) -> Result<()> { | |||
}; | |||
let meili = check_meili(&config).await; | |||
let opencast_sync = check_opencast_sync(&config).await; | |||
let oc_external_api = check_external_api(&config).await; |
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.
opencast_
vs. oc_
triggers me slightly. 😐
let opencast_sync = check_opencast_sync(&config).await; | ||
let oc_external_api = check_external_api(&config).await; |
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's slightly unfortunate that both of these have to create an Opencast client. 🤔
pub(crate) fn external_api_node(&self) -> &HttpHost { | ||
self.external_api_node.as_ref().unwrap_or_else(|| self.unwrap_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.
This is the third instance of this pattern, at which point I personally would want a thing for it. 😅
We decided that none of the comments block this from merging. |
This allows admins to override which OC node is used for external API
operations. Previously,
sync_node
was used, which was not alwayscorrect.
Fixes #1216