Hacker News new | past | comments | ask | show | jobs | submit login

Unsolicited review of sync.sh ahoy!

The easy stuff:

- Non-exported variables are by convention lower_case, while exported variables are UPPER_CASE.

- Function names are by convention lower_case.

- There's a "quite" signature string and variable; should it be "quiet"?

- The lines like `local hosts=` can be simplified as `local hosts`. You can also join such lines, as with `export`, into `local foo bar baz …`.

- `[[` is generally considered more reliable than `[` (there are a bunch of posts about this on Stack Overflow and Unix Stack Exchange).

- Naming scripts .bash rather than .sh lets `shellcheck` inspect them without telling it which shell to verify against. You do check these files using `shellcheck`, right?

- Variable names like `list` and `tuple` are unhelpful; what do they actually contain? Also, they should probably be actual arrays rather than a space-separated string to avoid relying on word splitting, and to allow entries with whitespace in them.

The scary stuff:

- All the string packing and unpacking also makes me nervous. That's not simpler than using JSON/YAML, it's just a different kind of complexity.

- Reusing the same variable a bunch of times in the same function makes it really easy to end up with the wrong value at some point. Better to split stuff into more functions or rename the variables to make them single use.

- `set -o errexit -o pipefail` would be good to have for safety.

- `kill -9` anything[1]

- Why are some of the variables inside functions not local? They effectively end up being globals, which makes for really difficult debugging.

The good stuff:

- Using local variables everywhere.

- Descriptive error messages.

- Returning early in case of errors.

All in all, Bash is not a good language for any system of this size, no matter how diligently you program. The error handling isn't comparable to most mainstream languages, the data types are incredibly limited, and there's no native support of recursive languages like JSON. You've done a great job, but I'm afraid a system like this is doomed to failure from these facts alone.

  [1] https://mywiki.wooledge.org/ProcessManagement#I.27m_trying_to_kill_-9_my_job_but_blah_blah_blah...



Thanks for taking the time l0b0!

A lot of good points.

Coding in shell sure is a challenge, I try to avoid bashims where I can, that's why most scripts are .sh not .bash, since they run also with dash/ash (that's why preferring `[` over `[[`).

The only Bash requirement we have is the `podc` (pod compiler) since it parses YAML docs, I couldn't pull that one off without the more powerful Bash.

The code base as a whole absolutely needs testing and shellchecks all over the place to be labelled as mature in any sense.

Our other large shell project is Space.sh [1] where go overboard on testing, each module is tested under a bunch of distributions. [2] [3] Next step would be to do the same for Simplenetes.

Given that in shell that the only way to protect a variable from functions down the call stack accessing it is to redeclare/shadow it as "local" brings some murky waters.

BUT, isn't it amazing what is actually possible to do with Shell??

For me Simplenetes in the long run is not about it being written in Shell, I'm perfectly happy rewriting it in Rust if we get traction on it.

Simplenetes is primarily about having a simpler architecture of what a container cluster is and is not. I think many projects just get too complex because they want to fill every single use case out there, while the interesting part is saying no to things.

[1] https://github.com/space-sh/space [2] https://github.com/space-sh/space/tree/master/test [3] https://github.com/space-sh/space/blob/master/.github/workfl...


Very good reasons all round. Good luck, we need simpler solutions in the cloud space!




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: