From 1e92fadf257aab0019327f3dfe6ff2ab719d04d6 Mon Sep 17 00:00:00 2001 From: Paris Goldman-Smith Date: Tue, 25 Sep 2018 17:07:39 +0100 Subject: [PATCH 1/3] created refactor doc this doc outlines the solutions we've come up with to solve the circular dependency issues within our projects. --- docs/CFL-refactor.md | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 docs/CFL-refactor.md diff --git a/docs/CFL-refactor.md b/docs/CFL-refactor.md new file mode 100644 index 000000000..75cddfcf0 --- /dev/null +++ b/docs/CFL-refactor.md @@ -0,0 +1,48 @@ +# CFL Refactor (remove the circular dependancies) +Currently upgrading versions for our different projects is far more effort than it needs to be. So it has been discussed by members of the team that a refactor might be worth looking into. + + +## Old model +```mermaid +graph TD; +Portal-->RapidRouter; +RapidRouter-->Portal; +Portal-->AI:MMO; +``` + +Currently there is a circular dependency between Rapid-Router and the Portal, (see above diagram). This has lead to code that is messy in places, and has made upgrading package versions incredibly difficult. This has been an issue for a number of years it seems, with mentions to it [here](https://github.com/ocadotechnology/rapid-router/issues/688) and [here](https://github.com/ocadotechnology/rapid-router/issues/658) dating back to 2015. + +While this is a long and potentially challenging task, i feel it would make long term development and maintenance of the various repositories significantly easier. + +## Solutions + +### Refactor to new model + +```mermaid +graph TD; +CFLmodel-->AI:MMO; +CFLmodel-->RapidRouter; +AI:MMO-->Portal; +CFLmodel-->Portal; +RapidRouter-->Portal; +``` +The above diagram shows a new model with which to organise our current projects. This model should eliminate the currently existing circular dependancy and should prevent it from occurring in future. The main idea involves separating the models section from portal, and placing it within it's own repository if needed, (written as "CFLmodel" within this document). this CFLmodel would contain all the things the models currently do within the portal repository, this includes: Storing and retrieving data, validating data, relations between the data, and how the data can behave. + +This CFLmodel would optionally include common styles used throughout our projects, one notable example being the banner on the portal website. + +From this our games (Rapid-Router & AI:MMO) can simply import the models they need directly instead of going through portal. Then Portal itself can also simply import our models directly, and then also import AI:MMO & Rapid-Router (And any other game/app we may develop in future). + +With the setup, updating package versions becomes a much simpler process. As first we need to upgrade our models to the desired version, then we can upgrade AI:MMO and/or Rapid-Router, and finally once all of that has been upgraded we can upgrade Portal. + +In conclusion, this approach would require potentially a lot of refactoring of our codebases. However most of the work should just involve changing where the imports come from, and creating this need CFLmodel repository. + +### Dependancy injections + +This is the other proposed solution. it has 3 key concepts, which are as follows: + * Components do not know each other directly. + * Components specify external dependencies using some kind of a key. + * Some "superior instance" (the IoC container, for example) resolves the dependencies once for each component and hereby "wires" the components together. + +These are taken from [this](https://wiki.python.org/moin/DependencyInjectionPattern) python wiki page, i've also linked to a page explaining the idea in more detail [here](http://code.activestate.com/recipes/413268/). + +The advantage of this approach over the previous one, is that this will not require as significant a refactor of our code, I have also been told there is an existing example of this within AI:MMO which we can use as reference. From 1a86c0a9687d4356eb367359f80df4dc90ec3554 Mon Sep 17 00:00:00 2001 From: dent50cent <35925962+dent50cent@users.noreply.github.com> Date: Tue, 2 Oct 2018 11:39:36 +0100 Subject: [PATCH 2/3] Various changes and grammar fixes --- docs/CFL-refactor.md | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/docs/CFL-refactor.md b/docs/CFL-refactor.md index 75cddfcf0..c4c891066 100644 --- a/docs/CFL-refactor.md +++ b/docs/CFL-refactor.md @@ -1,5 +1,5 @@ # CFL Refactor (remove the circular dependancies) -Currently upgrading versions for our different projects is far more effort than it needs to be. So it has been discussed by members of the team that a refactor might be worth looking into. +Currently, upgrading our different projects is far more effort than it needs to be. So it has been discussed by members of the team that a refactor might be worth looking into. ## Old model @@ -10,13 +10,13 @@ RapidRouter-->Portal; Portal-->AI:MMO; ``` -Currently there is a circular dependency between Rapid-Router and the Portal, (see above diagram). This has lead to code that is messy in places, and has made upgrading package versions incredibly difficult. This has been an issue for a number of years it seems, with mentions to it [here](https://github.com/ocadotechnology/rapid-router/issues/688) and [here](https://github.com/ocadotechnology/rapid-router/issues/658) dating back to 2015. +There is a circular dependency between Rapid-Router and the Portal, (see above diagram). This has led to code that is messy in places and has made upgrading package versions difficult. This has been an issue for a few years, dating back to 2015. See past issues: here and here. -While this is a long and potentially challenging task, i feel it would make long term development and maintenance of the various repositories significantly easier. +Whilst this is a challenging task, it would make long-term development and maintenance of the various repositories significantly easier. -## Solutions +## Solution ideas -### Refactor to new model +### Option 1: Refactor to new model ```mermaid graph TD; @@ -26,23 +26,34 @@ AI:MMO-->Portal; CFLmodel-->Portal; RapidRouter-->Portal; ``` -The above diagram shows a new model with which to organise our current projects. This model should eliminate the currently existing circular dependancy and should prevent it from occurring in future. The main idea involves separating the models section from portal, and placing it within it's own repository if needed, (written as "CFLmodel" within this document). this CFLmodel would contain all the things the models currently do within the portal repository, this includes: Storing and retrieving data, validating data, relations between the data, and how the data can behave. +The above diagram shows a new model with which to organise our current projects. This model should eliminate the currently existing circular dependancy and should prevent it from occurring in future. The main idea involves separating the models section from portal, and placing it within its own repository if needed, (written as "CFLmodel" within this document). This CFLmodel would contain all the things the models currently do within the portal repository, this includes: Storing and retrieving data, validating data, relations between the data, and how the data can behave. This CFLmodel would optionally include common styles used throughout our projects, one notable example being the banner on the portal website. From this our games (Rapid-Router & AI:MMO) can simply import the models they need directly instead of going through portal. Then Portal itself can also simply import our models directly, and then also import AI:MMO & Rapid-Router (And any other game/app we may develop in future). -With the setup, updating package versions becomes a much simpler process. As first we need to upgrade our models to the desired version, then we can upgrade AI:MMO and/or Rapid-Router, and finally once all of that has been upgraded we can upgrade Portal. +With the setup, updating package versions would become a much simpler process. First, we would need to upgrade our models to the desired version, then we would be able to upgrade AI:MMO and/or Rapid-Router, and finally once that has been upgraded we can upgrade Portal. -In conclusion, this approach would require potentially a lot of refactoring of our codebases. However most of the work should just involve changing where the imports come from, and creating this need CFLmodel repository. +#### Pros: +* Easier to understand the structure for a new contributor +* Clearer legacy code +* Easier to debug +#### Cons: +* May take a long time to complete +* More work -### Dependancy injections +### Option 2: Dependancy injections -This is the other proposed solution. it has 3 key concepts, which are as follows: +This option has 3 key concepts, which are as follows: * Components do not know each other directly. - * Components specify external dependencies using some kind of a key. - * Some "superior instance" (the IoC container, for example) resolves the dependencies once for each component and hereby "wires" the components together. + * Components specify external dependancies using some kind of a key. + * Some "superior instance" (the IoC container, for example) resolves the dependancies once for each component and hereby "wires" the components together. -These are taken from [this](https://wiki.python.org/moin/DependencyInjectionPattern) python wiki page, i've also linked to a page explaining the idea in more detail [here](http://code.activestate.com/recipes/413268/). +These are taken from [this](https://wiki.python.org/moin/DependencyInjectionPattern) python wiki page, I've also linked to a page explaining the idea in more detail [here](http://code.activestate.com/recipes/413268/). -The advantage of this approach over the previous one, is that this will not require as significant a refactor of our code, I have also been told there is an existing example of this within AI:MMO which we can use as reference. + +#### Pros: +* Faster to implement +#### Cons: +* Doesn't fix legacy code issues +* Harder to debug From 408a1f6517ba60b81ef275f122179b51d98e277d Mon Sep 17 00:00:00 2001 From: Florian Aucomte <33633200+faucomte97@users.noreply.github.com> Date: Fri, 12 Oct 2018 16:48:02 +0100 Subject: [PATCH 3/3] Update minikube status check (#846) * Update logic to check whether minikube is already running. * Updated except to check for CalledProcessError. * Merge branch 'master' into minikube-run-start --- aimmo_runner/minikube.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/aimmo_runner/minikube.py b/aimmo_runner/minikube.py index fd32101ef..7a333efd0 100644 --- a/aimmo_runner/minikube.py +++ b/aimmo_runner/minikube.py @@ -1,6 +1,8 @@ #!/user/bin/env python from __future__ import print_function +from subprocess import CalledProcessError + import docker import kubernetes import os @@ -74,10 +76,10 @@ def start_cluster(minikube): Starts the cluster unless it has been already started by the user. :param minikube: Executable minikube installed beforehand. """ - status = run_command([minikube, 'status'], True) - if 'minikube: Running' in status: + try: + run_command([minikube, 'status'], True) print('Cluster already running') - else: + except CalledProcessError: run_command([minikube, 'start', '--memory=2048', '--cpus=2'])