Add Preconfigure Step #22
Conversation
| @@ -87,7 +87,8 @@ class ConstellationContainer: | |||
|
|
|||
| def __init__(self, name, image, args=None, | |||
| mounts=None, ports=None, environment=None, configure=None, | |||
| entrypoint=None, working_dir=None, labels=None): | |||
| entrypoint=None, working_dir=None, labels=None, | |||
| preconfigure=None, network="none"): | |||
There was a problem hiding this comment.
What is up with this network change? Seems we'd not want to have things on the none network by default?
There was a problem hiding this comment.
Ah, I see, this is to deal with the odd dance below. Is the __init__ method ever not given "none" here? could just set that to "none" below?
There was a problem hiding this comment.
so I had to change this because of the wodin proxy, I need to specify the network for it and not use this "none" network trick because it would sometimes start and fail to find one of the wodin sites (since they have already been disconnected from the "none" network in that faff below), we could sleep the proxy for a bit but this is just a more direct way I think
constellation/constellation.py
Outdated
| entrypoint=self.entrypoint, | ||
| working_dir=self.working_dir, | ||
| labels=self.labels) | ||
| cl.images.pull(str(self.image)) |
There was a problem hiding this comment.
I don't think we want this pull here, it can be quite slow and assumes network access. Ensuring that the expected containers are present is better done elsewhere
There was a problem hiding this comment.
trying to figure out exactly where to do this, shall I just do a verification in the ConstellationContainer initialiser? that feels like a natural place to do it
There was a problem hiding this comment.
def ensure_image(name):
cl = docker.from_env()
try:
cl.images.get(name)
except docker.errors.ImageNotFound:
print(f"Pulling {name}")
cl.images.pull(name)
perhaps use a utility like that? Or punt for now and just make sure that CI has the images we need before it runs?
There was a problem hiding this comment.
yep was gonna do that! sorry badly worded, my question was more where to stick this utility? in __init__ of ConstellationContainer? just after we create the redis constellation containers in the tests that were failing? i think it should be in the __init__ of constellation container because the start function assumes image is pulled
There was a problem hiding this comment.
probably reasonable to put it just above any container.run call
There was a problem hiding this comment.
as discussed in standup, for now I have only put it in the tests that need redis image
This is required for the wodin deploy tool. Wodin containers cannot start successfully until they have the site configs inside of them however there was no way for us to create the containers and
docker cpthe files into the containers before the containers actually ran.I have added a preconfigure step to constellation that first creates the containers without running them and then runs them after preconfigure.
Also I needed to specify the network for wodin proxy container since it would try to look for the wodin containers immediately when it is put on the "none" network (this is the default way in constellation) however some of the wodin containers are connect to the actual wodin network so it fails. So I have included the ability to specify a network straight off the bat as well.
I have also had to change comparing types like
type(x) == ytoisinstance(x, y)due to linting issues with updated pycodestyle and added a pull step before the container creation so that the image is always there when the container is about to be created.The ticket for replacing the two vault tests: https://mrc-ide.myjetbrains.com/youtrack/issue/RESIDE-351/Vault-login-testing-with-GitHub-authentication