Conversation
labisso
left a comment
labisso
left a comment
There was a problem hiding this comment.
The overall approach seems fine, aside from the comments/questions I added.
In general do you feel the existing tests are sufficient to cover these changes? I might be missing something, but I don't see any that create more than one blockade in a single deployment, so that seems like a gap.
| global DATA_DIR | ||
| DATA_DIR = data_dir | ||
| BASE_BLOCKADE_DIR = os.path.join(DATA_DIR, ".blockade") | ||
| REST_STATE_FILE = os.path.join(BASE_BLOCKADE_DIR, "rest_state.yaml") |
There was a problem hiding this comment.
I think you need to declare these two as global if the changes are to stick. However, is there a reason to use global vars at all? Perhaps all three of these vars could be on the class instead?
| BLOCKADE_CONFIGS[name] = config | ||
| rest_state = BlockadeManager.read_rest_state() | ||
| rest_state[name] = config | ||
| BlockadeManager.write_rest_state(rest_state) |
There was a problem hiding this comment.
I'm not too familiar with Flask - are there multiple request handler threads? If so this behavior seems racy. Concurrent requests could step on each other. delete_config too.
2 participants
No description provided.