feat(192): added env expansion to the log_dir field of config::Config and .pem files used for SASL

[daneofmanythings]

This is a possible implementation of issue #192. I have also Attempted to eliminate the possibility of a None field for the log_dir path. That Optional had some trickle down effects the cluttered up the code. Let me know how it looks!

I had some thoughts about the validation process for the config. I see that the logging directory is being created deep within the creation of LoggerInner. My first intuition was to validate the path in config.validate, but that would take more of a refactor for something that may be out of scope for that function. I was thinking that an invalid path given as part of the config, whether optional or not, is invalid state. I would not want to continue to use the current session with an invalid config field and would prefer a crash with an error message. What do you think about that?

EDIT 1: I will fix the CI failures on the next iteration of the PR :) The tests all passed when I ran cargo test ., so I need to figure out this toolchain better.

[osa1]

Thanks @daneofmanythings.

EDIT 1: I will fix the CI failures on the next iteration of the PR :) The tests all passed when I ran cargo test ., so I need to figure out this toolchain better.

Just run cargo test, without the ..

I had some thoughts about the validation process for the config

Is this necessary for this feature? If not, let's consider validation of invalid paths in config separately.

[daneofmanythings]

I have made the required changes and it is now passing all tests! I am uncertain about the panic! i have in utils::expand_path. Is that how this function should handle this error? I read through the feedback on the previous PR, and I believe this accomplishes that.

[osa1]

We should think about where to allow shell expansion. Technically we could allow everywhere, e.g. even port: $OFTC_PORT could work. I'm not sure if it's a good idea though.

At the very least, we should also support

    sasl:
      pem: ~/certs/my.pem

[daneofmanythings]

I have reduced the pr to the essentials: there is an expand_path function in config.rs which uses shellexpand and is called in module to expand .pem files used for sasl and log_dir.

[osa1]

I think this looks mostly right now but error handling the how where we do shell expansion don't seem quite right yet.

I looked at the existing error handling. We sometimes print and exit, other times show the error in the TUI. It seems to depend on whether we consider the erorr critical or not.

• Unable to parse config: print errors to stdout and exit
• Unable to run password commands: same
• Unable to read PEM file: show error in TUI and continue without SASL auth

I don't understand why password command failure is considered critical here, but SASL auth issues are not. I think those two need to be consistent as they're both about authentication.

We also have to decide how to handle any errors during shell expansion. What we cannot do is panicking as we do now as it's inconsistent with how other errors are reported, and it doesn't let you handle the errors at the call sites. Even if we wanted to panic, we'd do it in main after catching the errors.

My suggestion is: we should print errors to stdout and exit in all cases. I kinda doubt you still want to run somehow if there's an error in your config. We should consider all errors as critical.

As to where to do shell expansion, I suggest a new function in config similar to read_passwords, that visits the fields where we expand shell variables and updates them. I think (but I'm not sure), it should be called before read_passwords, in main, so that password commands can refer to shell variables or use ~. Error reporting should be the same as read_passwords error reporting: print to stdout, return None. The caller would then exit(1) on error.

(We could also consider printing errors to stderr, but let's consider that separately.)

[osa1]

I'm not sure if it makes sense to do shell expansion during this conversion. See my top-level comment.

[osa1]

I think this impl should go. The try_from is used in one place, and it's actually used as try_into. There's no polymorphic use cases that needs a TryFrom<...> and we want to pass ClientSASLAuth, so this adds unnecessary overloading and makes the code more difficult to read for no reason.

We should add something like ClientSASLAuth::into_client_auth or similar that does the same thing.

[daneofmanythings]

Do you want this as a part of this PR? Seems unrelated.

[daneofmanythings]

Thank you for the detailed feedback! I'm afraid I need a little more clarification. I am modelling the new expand_field method off of read_passwords as you suggested. The signature of read_passwords is confusing me a little. It returns an Option<Config<String>> where the None path is to indicate an error has happened. Isn't this what Result<...> is for? I am also not seeing the exit point where that function will return None, so it seems to me that it always returns Some even if there is a failure coming from run_command (which also uses the Some path of an Option to report an error). How should I be thinking about these error reporting channels?

[osa1]

@daneofmanythings the None returns happen implicitly at the ? use sites, e.g.

tiny/crates/tiny/src/config.rs

Line 347 in 41225c1

Some(PassOrCmd::Cmd(cmd)) => Some(run_command("server password", &addr, &cmd)?),

Here the run_command(...)? returns None from the current function when run_command(...) returns None.

Option and Result can both be used for error returning. If you have an error value to return you use Result<..., MyError>. If you don't have an error value to return (you just want to signal a failure), you can use Result<..., ()>, or Option<...> which has the same information. Here we print errors already so the call site doesn't do any error handling, so we use Option instead of Result to signal an error.

[daneofmanythings]

Thank you for the clarification. Implementing now.

[daneofmanythings]

Okay, ready for review!

[osa1]

Thanks, I think this is mostly good.

Any thoughts on how to test this?

[daneofmanythings]

Thanks for the detailed feedback. Addressed everything with one deviation. Right now, the error bubbles up to the call sight instead of being converted to a bool. This greatly simplified the code.

As for testing, my instinct is to mock Config with a trait like ValidatableConfig. Then that can be used to isolate the behavior from the side effects in a parameterized test. It is a significant amount of code relative to the size of the commit, so I have not included it yet. Is that how you would approach it?