Fix kubernetes mode

[TheseusGrey]

Description

After merging both the python 3 and the c-002 branches together, the workers stopped being created in Kubernetes mode despite work fine in their respective branches. This was due to a small bug in the python 3 code and has now been fixed.

---

This change is 

[faucomte97]

Reviewed 2 of 3 files at r1, 2 of 4 files at r2, 7 of 8 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dent50cent)

---

aimmo-game/simulation/game_runner.py, line 64 at r3 (raw file):

    async def update_simulation(self, player_id_to_serialised_actions):
        LOGGER.debug(player_id_to_serialised_actions)

Remove logger please :)

[TheseusGrey]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97)

---

aimmo-game/simulation/game_runner.py, line 64 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove logger please :)

Done.

[OlafSzmidt]

Reviewed 2 of 3 files at r1, 2 of 4 files at r2, 7 of 8 files at r3.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @OlafSzmidt, @faucomte97, and @dent50cent)

---

aimmo-game/tests/functional/test_damage_pickups_and_effects.py, line 27 at r3 (raw file):

        pickup_created = DamageBoostPickup(self.cell)
        self.cell.pickup = pickup_created
        loop = asyncio.get_event_loop()

Move this and every other line below into setUP instead?

---

aimmo-game/tests/functional/test_health_pickups_and_effects.py, line 33 at r3 (raw file):

        self.cell.pickup = HealthPickup(self.cell)

        loop = asyncio.get_event_loop()

Repetition in setUP for event loops

[TheseusGrey]

Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @OlafSzmidt and @faucomte97)

---

aimmo-game/tests/functional/test_damage_pickups_and_effects.py, line 27 at r3 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Move this and every other line below into setUP instead?

Done.

---

aimmo-game/tests/functional/test_health_pickups_and_effects.py, line 33 at r3 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Repetition in setUP for event loops

Done.

[faucomte97]

Reviewed 1 of 3 files at r4.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @OlafSzmidt and @faucomte97)

[OlafSzmidt]

Reviewed 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[mrniket]

Reviewable status: 10 of 24 files reviewed, 1 unresolved discussion (waiting on @OlafSzmidt, @faucomte97, and @mrniket)

a discussion (no related file):
WIP (feel free to resolve this whenever it's ready for review again)

---

[TheseusGrey]

Reviewable status: 10 of 28 files reviewed, 1 unresolved discussion (waiting on @OlafSzmidt, @faucomte97, and @mrniket)

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

WIP (feel free to resolve this whenever it's ready for review again)

Done.

---

[mrniket]

Reviewed 10 of 14 files at r5, 7 of 8 files at r6.
Reviewable status: 27 of 28 files reviewed, 7 unresolved discussions (waiting on @dent50cent)

---

.travis.yml, line 39 at r6 (raw file):

        apt:
          packages:
            - docker-ce

does this make it work correctly with buildkit or do we need to set the environment variable as well?

---

.travis.yml, line 93 at r6 (raw file):

        apt:
          packages:
            - docker-ce

I don't think the Javascript tests need the docker addon

---

.travis.yml, line 129 at r6 (raw file):

        apt:
          packages:
            - docker-ce

the deploy step shouldn't need the docker add-on either

---

aimmo-game-creator/hooks/build, line 2 at r6 (raw file):

#!/bin/bash
docker build --target runner -t $IMAGE_NAME .

new line

---

aimmo-game-worker/hooks/build, line 2 at r6 (raw file):

#!/bin/bash
docker build --target runner -t $IMAGE_NAME .

new line

---

aimmo-game/hooks/build, line 3 at r6 (raw file):

#!/bin/bash

docker build --target runner -t $IMAGE_NAME .

new line

---

aimmo_runner/docker_scripts.py, line 19 at r6 (raw file):

def create_docker_client(raw_env_settings):

have we tested minikube with build kit as well?

[mrniket]

Reviewed 1 of 8 files at r6.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dent50cent)

[TheseusGrey]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mrniket)

---

.travis.yml, line 39 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

does this make it work correctly with buildkit or do we need to set the environment variable as well?

They aren't using buildkit on Travis (see latest aimmo worker travis tests) . However it would still run the tests correctly despite that, the main use for buildkit is when we need to ignore the test stage. but i can set an environment variable to see if that will enable it

---

.travis.yml, line 93 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

I don't think the Javascript tests need the docker addon

Removed it.

---

.travis.yml, line 129 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

the deploy step shouldn't need the docker add-on either

Removed it.

---

aimmo-game-creator/hooks/build, line 2 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

new line

Done.

---

aimmo-game-worker/hooks/build, line 2 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

new line

Done.

---

aimmo-game/hooks/build, line 3 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

new line

Done.

---

aimmo_runner/docker_scripts.py, line 19 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

have we tested minikube with build kit as well?

No, I updated the docker scripts so that it adds the buildkit env variable when creating the client for building the images for Minikube. It should now use buildkit in both cases.

[mrniket]

Reviewed 5 of 5 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dent50cent)

---

.travis.yml, line 39 at r6 (raw file):

Previously, dent50cent wrote…

They aren't using buildkit on Travis (see latest aimmo worker travis tests) . However it would still run the tests correctly despite that, the main use for buildkit is when we need to ignore the test stage. but i can set an environment variable to see if that will enable it

If it's as easy as setting an environment variable I would say it's worth a try, as it could lead to confusion later on

---

aimmo_runner/docker_scripts.py, line 19 at r6 (raw file):

Previously, dent50cent wrote…

No, I updated the docker scripts so that it adds the buildkit env variable when creating the client for building the images for Minikube. It should now use buildkit in both cases.

if we are setting it for both cases of the if statement then we can just set the buildkit env variable after the if statement :)

also does it work with minikube with this setting?

---

aimmo_runner/docker_scripts.py, line 51 at r7 (raw file):

        client = create_docker_client(raw_env_settings)
    else:
        client = create_docker_client('driver does not support')

can we use a boolean here instead? maybe it makes sense to calc raw_env_settings in the create_docker_client method?

[TheseusGrey]

Reviewable status: 26 of 28 files reviewed, 3 unresolved discussions (waiting on @mrniket)

---

.travis.yml, line 39 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

If it's as easy as setting an environment variable I would say it's worth a try, as it could lead to confusion later on

i added the env variable, however i it turns out travis is using docker 17.09 and so it doesn't support buildkit, i think it might be because of where we install it from?

---

aimmo_runner/docker_scripts.py, line 19 at r6 (raw file):

Previously, mrniket (Niket Shah) wrote…

if we are setting it for both cases of the if statement then we can just set the buildkit env variable after the if statement :)

also does it work with minikube with this setting?

I tested it in Minikube and everything seemed to work fine.

---

aimmo_runner/docker_scripts.py, line 51 at r7 (raw file):

Previously, mrniket (Niket Shah) wrote…

can we use a boolean here instead? maybe it makes sense to calc raw_env_settings in the create_docker_client method?

Done.

[TheseusGrey]

Reviewable status: 26 of 28 files reviewed, 3 unresolved discussions (waiting on @mrniket)

---

.travis.yml, line 39 at r6 (raw file):

Previously, dent50cent wrote…

i added the env variable, however i it turns out travis is using docker 17.09 and so it doesn't support buildkit, i think it might be because of where we install it from?

follow-up, I removed the docker-ce addon section in the travis.yml, and then checked the travis logs for building the image and it built using buildkit, it looks like the apt addon gets docker 17.09 but we already had 18.09 when we specify docker in the sevices

[mrniket]

Reviewed 2 of 2 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved

---

.travis.yml, line 39 at r6 (raw file):

Previously, dent50cent wrote…

follow-up, I removed the docker-ce addon section in the travis.yml, and then checked the travis logs for building the image and it built using buildkit, it looks like the apt addon gets docker 17.09 but we already had 18.09 when we specify docker in the sevices

awesome!

[mrniket]

Reviewable status: complete! all files reviewed, all discussions resolved