-
Notifications
You must be signed in to change notification settings - Fork 9
Volume mount package root hack for nifi #159
Conversation
…te config data in the package (for nifi operator testing)
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.
Left a counter-proposal
for (target_path, volume) in volume_mounts { | ||
// This is a hack for the NiFi operator. We need the volume mounts for NiFi to point to |
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.
I think we could make this a little bit less hacky by actually using the template renderer instead of string substitution here.
The current implementation would miss {{ packageroot }}
for example, which is still a valid Handlebars reference.
Something along these lines should have a similar effect:
let joined_target_path = pod_state.get_service_package_directory().join(
CreatingConfig::render_config_template(&template_data, &target_path).unwrap(),
);
{{packageroot}}/tmp" should be rendered to for example "/opt/stackable/packages/kafka-2.6.0/tmp"
Since PathBuf::join() resolves absolute paths absolutely this would lead to
"/opt/stackable/servicedirectory".join("/opt/stackable/packages/kafka-2.6.0/tmp") -> "/opt/stackable/packages/kafka-2.6.0/tmp"
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.
Yes thank you, that looks much better :)
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.
LGTM
I've rigged this quick'n dirty test, which doesn't really test the code itself, just the principle because the code was written more or less in an untestable fashion by me back in the day. We'll need to refactor that at some point and then add proper tests.
#[rstest]
#[case("{{packageroot}}/test", "/opt/stackable/packages/test")]
#[case("/test", "/test")]
#[case("test", "/etc/stackable/config/test")]
fn test_create_config_path(#[case] input: &str, #[case] expected_output: &str) {
let mut template_data: BTreeMap<String, String> = BTreeMap::new();
template_data.insert(
"packageroot".to_string(),
"/opt/stackable/packages".to_string(),
);
let config_dir = PathBuf::from_str("/etc/stackable/config").unwrap();
let output = config_dir
.join(&CreatingConfig::render_config_template(&template_data, input).unwrap());
let output = output.to_string_lossy();
assert_eq!(output, expected_output);
}
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.
LGTM
This is just a hack and should be removed after we find a better solution for nifi (e.g. with package and bootstrap script adaptation)