search

planetarium / NineChronicles.Headless

A headless node of NineChronicles game network, powered by Libplanet.
https://planetarium.github.io/NineChronicles.Headless/
GNU Affero General Public License v3.0
36 stars 38 forks source link

add `--consensus-propose-second-base` cli parameter #2467

Closed eseiker closed 5 months ago

eseiker commented 5 months ago

resolves #2441

This PR introduces --consensus-propose-second-base cli parameter to customize ContextTimeoutOption.ProposeSecondBase in LibplanetNodeService

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

eseiker commented 5 months ago

Passing it as an argument is fine, but it would be nice to override it with an environment variable.

It seems to work also with env vars such as Headless__ConsensusProposeSecondBase=4 dotnet run --project NineChronicles.Headless.Executable🙂

Oh it seems giving both env var and cli option leads to cli option win lol 😂 that's weird behavior... However it seems need some extra work to fix the behavior, to be another issue or PR IMO

moreal commented 5 months ago

Oh it seems giving both env var and cli option leads to cli option win lol 😂

It's right. Step by step:

  1. Load Configuration from appsettings.json https://github.com/planetarium/NineChronicles.Headless/blob/333d6a2593654d229032c11cf6c26bf2b9314351/NineChronicles.Headless.Executable/Program.cs#L241-L241
  2. Overrides Configuration with environment variables https://github.com/planetarium/NineChronicles.Headless/blob/333d6a2593654d229032c11cf6c26bf2b9314351/NineChronicles.Headless.Executable/Program.cs#L242-L242
  3. Overrides Configuration with cli options https://github.com/planetarium/NineChronicles.Headless/blob/333d6a2593654d229032c11cf6c26bf2b9314351/NineChronicles.Headless.Executable/Program.cs#L298-L309

so it's a natural behavior. I don't think it needs to be fixed further.