search

alces-software / adminware

A sandbox CLI for running commands remotely across nodes
1 stars 0 forks source link

Various tweaks and refactoring #171

Closed WilliamMcCumstie closed 5 years ago

WilliamMcCumstie commented 5 years ago

This PR contains various changes required for re-implementing the argument passing. The main user facing change is the complete removal of the current argument passing as recommended in #163. This will allow the arguments to be reworked in a future PR.

There has been a few naming tweaks following the standard python naming conventions. However this is by no means complete and will need further work. See issue #170

The final major change is updates to the database.Base model. It now contains shared code between all the database models:

  1. The table name is always the plural of the model name,
  2. The constructor has been moved to the base class (this was always the case but not used)
  3. All tables have an integer primary key
  4. The reconstructor has been baked into the Base
WilliamMcCumstie commented 5 years ago

This is the python standard as documented in: https://www.python.org/dev/peps/pep-0008/#descriptive-naming-styles

And if you really insist on list_ over list_groups that's good with me laughing

There is a lot of boiler plate code that is required for every database table (e.g. the table name, primary key, created_date). Currently this setup is spread across all the models which can lead to inconsistencies. Instead, these changes start to enforce a framework on how the models link together (broadly based on rails conventions).

please could you explain the motivation behind changing to how we interact with the DB, what is it achieving? This I just want to know for my own benefit

Whilst making db changes without migrations isn't ideal, in this case it can't be avoided w/o significant overhead. There is going to be another PR that introduces a shell_variables table which will be a breaking changing. This change has been approved by the production team. In the meantime, we should try and make all the breaking changes at once.

are you sure we'll be alright with changing the structure of the DB tables this extensively given our current lack of any migration support? Will this cause issues with backwards compatibility for existing installations?

DavidMarchant commented 5 years ago

Ok then, cheers for the response! If the migrations are set to be alright & there's a fix for the comment in the pipeline this is good for my end! Merge it through