Duplicate commands + checks?

[edgar-vincent]

Hi,

Thanks a lot for this very useful script.

I noticed that there were duplicate command calls:

https://github.com/r3nor/mullvad-tailscale/blob/1784d3139274e24136f7748a4061c4dfffc6294c/mtc#L136 and https://github.com/r3nor/mullvad-tailscale/blob/1784d3139274e24136f7748a4061c4dfffc6294c/mtc#L139

Also, is this needed https://github.com/r3nor/mullvad-tailscale/blob/1784d3139274e24136f7748a4061c4dfffc6294c/mtc#L153 ?

Thanks.

[edgar-vincent]

Also, the script should probably call exit with an error if either tailscale up or mullvad connect fails (because, for instance, they are not set up properly), otherwise the network might end up in a broken state (no DNS resolution in my case).

[edgar-vincent]

Same thing for nft. Since nftables applies the configuration atomically, it fails completely if any of its evaluations fail (it did in my case for reasons I'll describe in another issue). The script should probably exit and cleanup in that case.

[r3nor]

Have you been able to test the new version?

[edgar-vincent]

I have started debugging it, but haven't finished. It is quite a bit better, but there are a few things that need changing. I'll make a PR when I can.

[edgar-vincent]

Still working on it, there is a lot of reformatting to do. A few general pieces of advice:

• Try and keep the code, the documentation and the printed messages concise. The current version of the script and README is too verbose (also, don't use too many new lines. It is good to have empty space, but remember this can be run in TTYs or small terminals where space is expensive.
• Try and be consistent in the way messages are printed (verb tenses, punctuation, etc.); announce what the script is doing before it is doing it, not after. It is good to be helpful, but don't drown the user with information: stick with the essential info.
• Check for potential errors, all the time. You cannot expect everything to always work as you intend it to, and you don't want to leave the user's system in a broken state. This especially applies to user input/custom config.
• Expect external commands to do what they say they do. Temporary hacks are fine, but there shouldn't be too many of them.
• Don't duplicate code which can be refactored.
• Try and be consistent in your coding style (echo vs printf, [ vs [[, etc.). It improves readability for contributors.
• Longs pipes can be easier to read, however it is often better to limit the number of external commands you call.

This is going to be very useful script!

[r3nor]

Closing as it should be addressed with the latest version. Thank you @edgar-vincent !

[edgar-vincent]

Thanks for merging. The duplicate commands still need to be looked at though, but maybe it requires a specific issue.