chore(cli): configure remaining arguments via .env file

[thedevbirb]

Closes #458. This PR allows to configure the remaining CLI arguments using the .env file. I hope I haven't left some options out! :)

I have reviewed the changes by playing around with the .env file and see if the options were set properly, but I'm not very familiar with the codebase yet so I apologise in advance for any hiccup.

Small q: is there some formatting rule I need to enable? I think I'm running vanilla rustfmt but imports were consistently re-organised so I had to turn it off.

[CLAassistant]

All committers have signed the CLA.

[xgreenx]

Small q: is there some formatting rule I need to enable? I think I'm running vanilla rustfmt but imports were consistently re-organised so I had to turn it off.

We are using nightly formatter=)

[xgreenx]

Could you also update CHANGELOG.md please?=)

[xgreenx]

Also, it would be nice if you added a unit test that verifies that all arguments can be configured via the .env file.

[thedevbirb]

Also, it would be nice if you added a unit test that verifies that all arguments can be configured via the .env file.

Sorry to get back only now. I'm trying to do that but I'm wondering what's the best approach given there are a lot of arguments and I'd like some feedback if possible. I've seen there is a nice procedural macro test_case, used for example here:

fuel-core/bin/fuel-core/src/cli/run/consensus.rs

Lines 80 to 88 in 12cb17f

	#[test_case(&[] => Ok(Trigger::Instant); "defaults to instant trigger")]
	#[test_case(&["", "--poa-instant=false"] => Ok(Trigger::Never); "never trigger if instant is explicitly disabled")]
	#[test_case(&["", "--poa-interval-period=1s"] => Ok(Trigger::Interval { block_time: StdDuration::from_secs(1)}); "uses interval mode if set")]
	#[test_case(&["", "--poa-instant=true", "--poa-interval-period=1s"] => Err(()); "can't set interval and instant at the same time")]
	fn parse(args: &[&str]) -> Result<Trigger, ()> {
	Command::try_parse_from(args)
	.map_err(|_| ())
	.map(|c| c.trigger.into())
	}

This can be used also for environment variables as well, like so:

    #[test_case(&[] => Ok(Trigger::Instant); "defaults to instant trigger")]
    #[test_case(&["", "--poa-instant=false"] => Ok(Trigger::Never); "never trigger if instant is explicitly disabled")]
    #[test_case(&["POA_INSTANT=FALSE", ] => Ok(Trigger::Never); "ENV never trigger if instant is explicitly disabled")]
    #[test_case(&["", "--poa-interval-period=1s"] => Ok(Trigger::Interval { block_time: StdDuration::from_secs(1)}); "uses interval mode if set")]
    #[test_case(&["POA_INTERVAL_PERIOD=1s", ] => Ok(Trigger::Interval { block_time: StdDuration::from_secs(1)}); "ENV uses interval mode if set")]
    #[test_case(&["", "--poa-instant=true", "--poa-interval-period=1s"] => Err(()); "can't set interval and instant at the same time")]
    #[test_case(&["POA_INSTANT=TRUE", "POA_INTERVAL_PERIOD=1s"] => Err(()); "ENV can't set interval and instant at the same time")]
    fn parse(args: &[&str]) -> Result<Trigger, ()> {
        Command::try_parse_from(args)
            .map_err(|_| ())
            .map(|c| c.trigger.into())
    }

It would still be a bit noisy considering all the arguments of the run command, including nested fields such as P2PArgs etc. but it is doable.

I've also noticed that using reading from an environment variables file and using Command::parse() in tests isn't really ideal because if I run the single test then its name is set as an argument to the process, and clap tries to parse it and crashes.

[MitchTurner]

@thedevbirb

Sorry about the slow response. We appreciate the contribution.

I'd like some feedback if possible

What kind of feedback are you looking for?

I've seen there is a nice procedural macro test_case, used for example here:

I would encourage you to not force existing tests to fit your needs. Feel free to write some new tests with or without their own test_cases. If it's redundant, we can DRY it up later, but that would be a bonus.

I run the single test then its name is set as an argument to the process, and clap tries to parse it and crashes.

Having a hard time understanding the problem described here.

You can look to see if it's possible to use a tempfile to set up the .env file. If the path to he .env file is hardcoded it might be difficult. But that's the standard way of doing file IO inside tests since it gets cleaned up automatically when dropped from scope. If that won't work, we can operate under the assumption that things added to the .env file are loaded into the environment and std::env::set_var will be a good enough test.

[thedevbirb]

Thanks for the response! I'll try my best to clarify some points:

I run the single test then its name is set as an argument to the process, and clap tries to parse it and crashes.

Having a hard time understanding the problem described here.

Here is an example: suppose I'm in bin/fuel-core/src/cli/run/consensus.rs and I have the following test

#[cfg(test)]
mod tests {
    use super::*;
    use clap::Parser;

    #[derive(Debug, Clone, Parser)]
    pub struct Command {
        #[clap(flatten)]
        trigger: PoATriggerArgs,
    }

    #[test]
    fn parse_from_env() {
        std::env::set_var("POA_INTERVAL_PERIOD", "2s");
        Command::parse();
    }
}

The test may fail depending on how you run it, which I think is a bit undesirable: if you run it with cargo test from the project root directory than it works good (therefore ok for CI purposes I guess) but it breaks the moment you want to run it individually either by specifying the name of the test (e.g. cargo test parse_from_env) or by relying on IDE features which under the hood do the same thing.
The reason is that clap's parse or try_parse will:

• look first at std::env::args_os before environment variables;
• find parse_from_env or whatever identifier has been attached to the test name as an argument;
• try to parse it according to the Command struct definition, failing to do so.

error: unexpected argument 'parse_from_env' found

In my opinion not being able to run test individually is not an ideal development experience but of course I cannot enforce this. With that said, I think the only way to avoid clap crashing on individual tests is by using parse_from which parses from an iterator, as done in the existing tests which leverage the test_case macro.

What kind of feedback are you looking for?

In general I wanted to know if you had some guidelines/strategy I need to follow before trying to tackle these unit tests considering also the constraints outline above, also because it may be a lot of manual work even with the test_case macro 😅.
The tempfile crate suggestion is great, thanks! I didn't know about that crate. However, getting environment variables from the .env file or std::env::set_var and using parse still has the edge case outlined above.

[xgreenx]

Sorry for the delay=D PR looks good, but it must be updated to the latest master. I will close PR for now just to reduce the number of PRs=) But you are welcome to open it gain on top of the master with link to this PR=)