feat: Take dependencies into account when sorting environment variables

[siegfriedweber]

Description

Take dependencies into account when sorting environment variables

For example, the environment variables

ENV1=reference to $(ENV2)
ENV2=some value
ENV3=some value

are sorted in this order when iterated or converted to a vector:

ENV2
ENV1
ENV3

Part of stackabletech/issues#866

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added

[NickLarsenNZ]

LGTM, but left a comment to check my understanding.

[Techassi]

I would go as far as and say it must be a valid regex.

Suggested change

	LazyLock::new(|| Regex::new(r"\$\$").expect("should be a valid regular expression"));
	LazyLock::new(|| Regex::new(r"\$\$").expect("static string must be a valid regular expression"));

[siegfriedweber]

changed in 9a3111d

"Should" is often used in these cases. If I write a unit test that is EXPECTED to panic, I use the attribute #[should_panic] even if I know that it MUST panic as long as nobody changes the code.

[Techassi]

There is no need to clone. We can consume value.

Suggested change

	let mut vec: Self = value.0.values().cloned().collect();
	let mut vec: Self = value.0.into_values().collect();

[Techassi]

Oh we borrow value above and need to hold onto it after we collect the values into the Vec.

This indicates that the design of EnvVarDependencyResolver is sub optimal.

[siegfriedweber]

EnvVarDependencyResolver resolves the dependencies and provides sort keys for a later sort. Which design would you prefer?

[Techassi]

I feel like this whole struct should be build around owning the EnvVarSet. This would get rid of a whole bunch of clones all over the place. This additionally makes sense, because it is only used in a context in which the underlying EnvVarSet is consumed.

[siegfriedweber]

I feel like this whole struct should be build around owning the EnvVarSet.

If EnvVarDependencyResolver would own the EnvVarSet, then it would not be available to the sorting algorithm anymore.

This additionally makes sense, because it is only used in a context in which the underlying EnvVarSet is consumed.

An EnvVarSet should be usable for several containers or role-groups. So, consuming it, does not feel right for me.

This would get rid of a whole bunch of clones all over the place.

The code could be written completely without clones. But every avoided clone makes the code a bit harder to read. The EnvVarSets are small and every EnvVar usually contains a short name and a short value. So the clones are cheap.

However, I changed the code to use references (05660fa). The IntoIterator for EnvVarSet still clones the EnvVars, because this would be the alternative:

impl IntoIterator for EnvVarSet {
    type IntoIter = vec::IntoIter<Self::Item>;
    type Item = EnvVar;

    fn into_iter(self) -> Self::IntoIter {
        let sorted_indices: Vec<usize> = {
            let dependency_resolver = EnvVarDependencyResolver::new(
                &self,
                ENV_VAR_DEPENDENCY_RESOLVER_MAX_RECURSION_DEPTH,
            );

            let mut indexed_env_vars: Vec<(usize, &EnvVar)> =
                self.0.values().enumerate().collect();
            indexed_env_vars
                .sort_by_cached_key(|(_, env_var)| dependency_resolver.sort_key(env_var));

            indexed_env_vars
                .into_iter()
                .map(|(index, _)| index)
                .collect()
        };

        let mut env_vars: Vec<Option<EnvVar>> = self.0.into_values().map(Some).collect();

        sorted_indices
            .into_iter()
            .map(|index| {
                env_vars[index]
                    .take()
                    .expect("sorting must yield each index exactly once")
            })
            .collect::<Vec<_>>()
            .into_iter()
    }
}

[Techassi]

What's the reason this needs to be public?

[siegfriedweber]

Doc tests can only access public functions. I wanted to test each function in isolation because that made it easier to achieve high test coverage. I know this isn't idiomatic, but Rust doesn't offer an alternative I'd like better.

[NickLarsenNZ]

What about pub(crate) (I assume that still doesn't give it enough visibility)?

[siegfriedweber]

pub(crate) is not sufficient for doc tests.

If we would own EnvVar, then e.g. referenced_env_vars would be a public function there and it would be accepted that it is tested in isolation. We could wrap EnvVar, but that would be overkill.