refactor topgrade.toml's remote_topgrade field to be structs for per-host configurations

[ssmendon]

I want to suggest some general feature

Topgrade should allow better customization of per-remote defaults. Right now, we have the following options:

• remote_topgrades (Vec<String>)
• remote_topgrade_path (String)
• ssh_arguments (String)

My proposal is to change the representation and deprecate the older version. Instead, we should use a struct representing a remote with its options. The user can specify inside topgrade.toml an array of tables using syntax like:

[[remotes]]
destination = "ssh://root@foo:1234"

[[remotes]]
destination = "ubuntu@bar"
ssh_arguments = "-F ~/.ssh/mycustomconfig -A"

[[remotes]]
destination = "baz"
topgrade_path = "/bin/topgrade"

Internally, it could be represented with a struct similar to:

pub struct ConfigFile {
  // -- snip --
  remotes: Option<Vec<RemoteInfo>>,
  // -- snip --
}

pub struct RemoteInfo {
  destination: String,
  ssh_arguments: Option<Vec<String>>, // use something like serde_with or implement a custom deserializer
  topgrade_path: Option<String>,
}

This has the advantage of allowing for easier extensibility and testing, since we can isolate RemoteInfo logic from parsing the configuration file, command-line arguments, etc.

As an example of the extensibility, it would be easy to support a feature request for a user-defined value for $SHELL by adding a configuration parameter to RemoteInfo and implementing the Default trait.

More information

I would be open to implementing this if there is general agreement on this proposal.

[s34m]

I would love this idea because this would open us ways to include new features easier and also would make implementing "95 for example easy. Also the config looks neat if we do it how you describe it. I'm 100% for it.

Edit: Note: but if we plan on implementing it it would be great if the original version would still be usable, maybe with a depreciation message when running.

[9999years]

Yeah, I support this but would very much like configurations to remain backwards-compatible with a warning. (There is already some code for doing that in config.rs.)

[9999years]

Related issue: #95

[ssmendon]

I'm working on implementing this. I wanted to solicit some feedback about my current approach to see if there's any objections/suggestions before making a bunch of code changes to support it.

Requirements

• Backwards-compatibility with deprecated options
• Enable "Array of Tables" TOML syntax for individual hosts
• Allow specifying unique per-host options
• Keep the ability to specify global options for hosts

Sample Configs

Here are some different TOML files we should be able to parse.

# This is the original, deprecated config style
remote_topgrades = ["toothless", "pi", "parnas"]
ssh_arguments = "-o ConnectTimeout=2"
remote_topgrade_path = ".cargo/bin/topgrade"

# This is an example of the same information
# with the new format
[remote]
ssh_arguments = ["-o", "ConnectTimeout=2"] # global, affects all unless overwritten
topgrade_path = ".cargo/bin/topgrade"

[[remote.hosts]]
destination = "toothless"
topgrade_path = ".local/bin/topgrade" # should override the common configuration

[[remote.hosts]]
destination = "pi"

[[remote.hosts]]
destination = "parnas"

# If both deprecated values are used with the new values
# the deprecated values are ignored outright
remote_topgrades = ["toothless", "pi", "parnas"]
remote_topgrade_path = ".cargo/bin/topgrade" # should be ignored, it's set in the global config options

[remote]
topgrade_path = ".local/bin/topgrade"

# We could implement a custom Deserializer
# to support this
[remote]
ssh_arguments = "-o ConnectTimeout=2" # Array<String> or String
hosts = ["toothless", "pi", "parnas"] # Array<Host> or String (where String is `destination`)

Design

I'm trying to be DRY. The original TOML file supports options only at the global level, but we want to re-specify those options on a per-host basis as needed.

I think that it would be a good idea to factor out the common fields for the remotes and use the #[serde(flatten)] attribute. The caveat is that #[serde(deny_unknown_fields)] isn't supported with #[serde(flatten)], and #[serde(deny_unknown_fields)] is used throughout config.rs.

We could also factor out the remote-specific deprecated options into another struct. That could be bubbled up into ConfigFile using #[serde(flatten)] as well.

Here's an example of that:

use serde::Deserialize;

pub mod remote {
  use serde::Deserialize;

  #[derive(Debug, Default, Deserialize)
  pub struct Remote {
    pub hosts: Vec<Host>,
  
    #[serde(flatten)]
    pub common: Common,
  }
  
  #[derive(Debug, Default, Deserialize)
  struct Host {
    pub destination: String,
  
    #[serde(flatten)
    pub common: Common,
  }
  
  #[derive(Debug, Default, Deserialize)
  struct Common {
    pub ssh_arguments: Option<Vec<String>>,
    pub topgrade_path: Option<Vec<String>>,
  }
  
  #[derive(Debug, Default, Deserialize)
  pub struct Deprecated {
    pub remote_topgrades: Vec<String>,
    pub ssh_arguments: Option<String>,
    pub remote_topgrade_path: Option<String>,
  }
}

#[derive(Debug, Default, Deserialize)
pub struct ConfigFile {
  // --snip--
  remote: Option<remote::Remote>,

  #[serde(flatten)]
  deprecated_remote_config: remote::Deprecated,
}

Are there any reservations about this approach?

[J3m5]

Hello Has there been any progress on this issue? It would be very helpul