[DEVOPS-690] Dhall-based multi-cluster configuration

[deepfire]

No description provided.

[domenkozar]

One possible improvement would be to make environment specific configuration a function of OS specific configuration, that way we can deduplicate them.

[deepfire]

@domenkozar, I totally agree -- and I think I have an idea, actually.. Since Bool values are the only things that can be compared in Dhall, we can pass them down from the .bat file: isWin64, isMacOS64, isStaging, isMainnet.

That would resolve a lot of duplication..

[domenkozar]

I don't think there's anything to really compare?

A sketchy example:

let
  os = ./macos.dhall
in ./staging.dhall os.suffix

where staging.dhall would be:

\suffix ->
{ configuration = { key = "staging_full_wallet_${suffix}" }

[domenkozar]

Boolean conditionals won't work well as soon as you need to add another cluster (testnet). We need to be easily to copy an existing environment and slightly modify it. With current implementation we'll have to nest booleans.

[deepfire]

Good work on such a big change.

Thank you!

Clarity

Thing that concerns me is that it seems more complicated than it was before -- and less easy to understand because there's another layer. There's lots of stuff added but not much removed.

I've added a README section covering the general area of install-time generation of runtime YAML configuration files.

How will daedalus developers start the wallet if the yaml files are missing? I don't see any readme about that.

I'm not 100% positive on how much of a problem that is -- since they can always use the scripts/build-installer-* build scripts.

As a longer-term solution we could factor the config generation as a subcommand of make-installer.

In the near term we probably want to balance the risks an absence of this extension introduces against the blocking of dependent PR's.

Security

Please do remove the command line argument for certificate password. Use the environment variable set by CI and document this variable in the command usage help text.

Done.

Haskell

In Haskell, why use fully qualified names where the conventional module abbreviations are fine?

It's a matter of preference -- the Dhall upstream code base uses that. I have pushed a style normalisation commit.

I would recommend using more of Turtle, not less because it's really good for these kind of scripts.

I generally agree, absolutely. We've discussed this a lot : -)

Instead of functions like unsafeTextToLine and lshowText, use Turtle's printf and nice formatting functions.

lshow does a transformation that I'm not sure Turtle provides. Do agree on the rest.

Types.hs: what is the Request type? It needs comments. DeployParams might be a better name.

Yes, clarified, but opted for ConfigRequest, since that's essentially what it is -- parameters that drive the configuration generator.

WindowsInstaller.hs: when implementing new functionality such as invoking npm, put it into new functions. Lots of small functions are easier to understand and work with rather than one snaking main.

Done.

Config file generation

The dhall files - where are the comments?

Good point. It's a lot of work to properly document all the parameters that drive cardano-launcher. It should be noted that the original configuration files don't have that inline documentation either.

Do you have a suggestion for a shorter-term solution, or you believe this is a blocking issue?

If so, I'll start documenting all of the options. that factor into the output configuration files.

When a yaml file is generated from a dhall file, this tells me a build tool should be used, not a custom program/script. nix-build would be good for this.

I generally agree, yes. Question of short/long term balance is open, once more.

It would be better to output yaml files and everything else to a clean temp directory, rather than mixing in with the code.

This is a question of rethinking of how the entire Daedalus build is currently organised, I believe.
Again, short v. longer term balance.

installer-clusters.cfg - what is this file and why? It needs a comment at the top.

The way it is used precludes inline documentation -- I've added that on top of the README, instead.

Building the installer

build-installer-unix.sh - this file should be shrinking, not growing with added logic. The --clusters argument should be handled in make-installer.

I generally agree, yes. Again, I'm ready to embark on this ASAP, but we need to unblock dependent work.

installer-dev-mode.bat - this file shouldn't be needed. Testing on windows should be no more complicated than stack exec make-installer

Ideally, yes -- infeasible without prerequisite refactoring work and further delays, though.

installers/default.nix - note: I also have changes to this file in devops-673 and will be moving dhall and djall-json into separate files generated by cabal2nix.

Noted.

Summary

It's a good change on the whole but legibility needs to be improved in my opinion.

Yes, thank you for your feedback!

[rvl]

Nice effort.

[deepfire]

@domenkozar, @nikolaglumac, what would be our plan for getting this into the master branch?

Should we leave it as-is, or should I create a rebased PR?

Maybe we don't need to bother, if we don't expect more releases to be made.

[nikolaglumac]

@deepfire we should follow the git-flow approach. I think this should be merged into master with the next release. Do you agree @domenkozar? Is there some reason why we should merge this to master now?

[domenkozar]

No reason to treat this as a hot fix.