Add Icinga Web module totp - #492
Conversation
dgoetz
left a comment
dgoetz
left a comment
There was a problem hiding this comment.
Looks good to me, I just have one of my typical nitpicking if you want to adjust for this.
| resource: totp_db | ||
| settings: | ||
| issuer: Icinga Web 2 | ||
| leeway: 15 |
There was a problem hiding this comment.
Should we use here and in arguments specs the same values? Should we add a hint on the default of 10 seconds and the restriction for leeway being 0 to 29 seconds? And perhaps also where the issuer is shown as you decided for the more user friendly name here?
There was a problem hiding this comment.
Adding the issuer and leeway to the argument_specs.yml comes with a problem: As soon as just one option to the config dictionary is defined via argument_specs, no other options are allowed unless they are also explicitly defined in argument_specs.
So we would need to define all possible options or none of them.
Specifying none allows for more flexibility, especially when it comes to new options or options for which the option name changes (e.g. renaming issuer → issuer_name).
When specifying all options, changes like these lead to more maintainance work.
I'm fine with documenting it and telling users about it but I would not put the options in argument_specs to avoid manual work and divergent configuration options between different versions of the module in the future.
Added a hint to the docs for now.
2 participants
No description provided.