search

NixOS / nixops

NixOps is a tool for deploying to NixOS machines in a network or cloud.
https://nixos.org/nixops
GNU Lesser General Public License v3.0
1.82k stars 363 forks source link

Database attributes defaulting to None creates lots of bugs #926

Open nh2 opened 6 years ago

nh2 commented 6 years ago

A common idiom in nixops MachineState subclasses is the initialisation of lots of fields to None. For example:

https://github.com/NixOS/nixops/blob/da336200c15d2aa145b0ddf29d5d5db5702d4f4f/nixops/backends/digital_ocean.py#L64

This results in lots of bugs. For example: #627, #490, #530, #925. These are just the ones related to auth token handling I found within tha last 10 minutes; I have found other ones in the past and still regularly get surprised by it.

Usually, code like https://github.com/NixOS/nixops/blob/da336200c15d2aa145b0ddf29d5d5db5702d4f4f/nixops/backends/digital_ocean.py#L114-L115 breaks loudly when self.auth_token isn't set; avoiding a bug. But because it's defaulted to None, it silently continues and a bug is introduced.

I think we should not set things to None and then rely on everybody not forgetting to call whatever magic state updating functions.

Instead, if possible, we should construct values with their correct values initially.

I don't know what exactly is responsible for this situation / how it came to be this way. I suspect it's the "self.something variables magically update the sqlite DB, much convenient" logic that nixops currently has instead of explicit separation of what is stored state and what isn't, but I haven't convinced myself of that yet.

ruhatch commented 5 years ago

I am running into this issue right now, specifically with the netmask attribute being null on this line. I'm not sure how to resolve that issue and definitely would like the whole backend to be more robust to this kind of error.