ffnord / mesh-announce

Discussion at #mesh-announce:irc.hackint.org and (separately) at
https://matrix.to/#/!MjLIHcALOcENXZWQlH:irc.hackint.org/$1547640760901FmKaD:matrix.eclabs.de
13 stars 45 forks source link

[FEATURE] multidomain-domainnames #59

Closed AiyionPrime closed 4 years ago

AiyionPrime commented 4 years ago

This implements the use of domaincode-files, which are simple json dictionaries holding batadv_device-names as keys and the related domain as their value.

If no or an empty json-file is provided, the code falls back to known behavior and returns the value of "-n" which was implemented in https://github.com/ffnord/mesh-announce/pull/55. If used this makes the value of '-n' the fallback value.

This PR fixes #56 and provides half of #53.

The idea of binding bat-devices and domaincodes is shamelessly stolen from fftrier: https://github.com/freifunktrier/mesh-announce/blob/fftr_mods/providers/nodeinfo/system/domain_code.py

The approach of a json was deliberately chosen in order not to spam the commandline-call of respondd.py with further repeating parameters.

AiyionPrime commented 4 years ago

Ah wait. We lack an entry in the readme file.

AiyionPrime commented 4 years ago

I'm fine.

TobleMiner commented 4 years ago

Hey, thanks for the patch!

I'm not sure about using a config file for the batman-device <-> domain association though. While the command line arguments have already gotten far too complex I don't feel comfortable pushing just this part to a conffile. I'd prefer to keep it part of the command line arguments for now. Maybe as -b <batman_iface>[:[<mesh_ipv4>][:<domain code>]]

AiyionPrime commented 4 years ago

The association itself should be watched carefully, as it goes straight against @genofire s efforts of possibly getting rid of batman and use babel instead, irrc. I'd rather communicate this configuration as a temporary necessity, until good things happen, like @mweinelt s rfc efforts to find a gluon wide solution for this, but realistically this will take quite some time.

I do not really agree with the concept of sticking to the pile of commandlinearguments; however if accepting the PR depends on it, I'll implement it for the domain-code.

If someone could provide an example of a working commandline call with two? bat-devices and their mesh_ipv4s in the syntax you provided, it would help me a lot.

-b <batman_iface>[:[<mesh_ipv4>]...]

AiyionPrime commented 4 years ago

I don't feel comfortable pushing just this part to a conffile. I'd prefer to keep it part of the command line arguments for now. Maybe as -b <batman_iface>[:[<mesh_ipv4>][:<domain code>]]

Hey @TobleMiner forget my last request for the commandlinecall, I think I found it in the code.

AiyionPrime commented 4 years ago

Well then, this implemented [:<domain code>]. -b interface and -n dom13 are (except the defaulting behavior for other domains) equal to -b interface::dom13. In this case the mesh_ipv4_override is not set for this interface.

This finally allows the domaincode-file to overwrite the default domain given by -n. And the -b :: flags to overwrite per interface on the fly.

Therefore it's: -n < -c < -b.

As last thing to allow a start for a proper config-file, I'd like to change the domaincode-file format, so that it's not a dict of domain-codes anymore, but a dict with one key called "domaincodes" and the current dict nested within as its value. This would allow to seamlessly rename the domain_code file as well the -c parameter to configfile later on, without breaking domaincode related stuff. What do you think about this proposition @TobleMiner , @mweinelt ?

AiyionPrime commented 4 years ago

@TobleMiner I think, I'm done. Review requested.

Currently testing in hanover. Works as intended.

TobleMiner commented 4 years ago

Hi,

I do not really agree with the concept of sticking to the pile of commandlinearguments; however if accepting the PR depends on it, I'll implement it for the domain-code.

Don't get me wrong here, I don't like it either. But I do prefer it over introducing a config file format that is not capable of representing all configuration options of mesh-announce in a proper structure. If we were to introduce the association file right now it would either create a breaking change as soon as a proper config file format is introduced or we would need to keep support for the file around for some time. I'll take it like that but be prepared for me kicking out the association file and replacing it by a proper config for the daemon.

I'll test the PR and merge it shortly

TobleMiner commented 4 years ago

LGTM, thanks for implementing!