Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mlbackend_python tests #113

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

dmonllao
Copy link

@dmonllao dmonllao commented Dec 5, 2019

No description provided.

base.yml Outdated
mlbackendpython:
image: "moodlehq/moodle-mlbackend-python:2.3.0-python3.7.5"
ports:
- "5000:5000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dmonllao,

  1. you've missed to add a new dependency to the webserver service
  2. you don't need to expose the TCP port 5000 unless for other purposes: when you use a compose, a dedicated network interconnects all the services within that compose i.e. mlbackendpython:5000 should be already available without mapping the port on the docker host too

HTH,
Matteo

@dmonllao
Copy link
Author

dmonllao commented Dec 6, 2019

Many thanks for the review Matteo. I've updated the patch, removing the port and moving the definition of the new image to phpunit-external-services.yml

@stronk7
Copy link
Member

stronk7 commented Jan 8, 2020

I'm a little bit worried about how we are going to manage different versions here whenever they diverge between branches or when moodle-docker is used for versions < 3.8.0 ... uhm...

@dmonllao
Copy link
Author

dmonllao commented Jan 9, 2020

any similar case in moodle-docker? (e.g. different pg/redis/ldap versions used for different moodle major branches). If MOODLE_DOCKER_WWWROOT must be set when the containers are initialised (https://github.com/moodlehq/moodle-docker/blob/master/bin/moodle-docker-compose#L4) we could modify bin/moodle-docker-compose to:

  1. Read the info in version.php
  2. Set up a hardcoded mapping array from Moodle major branches to latest tags in https://hub.docker.com/repository/docker/moodlehq/moodle-mlbackend-python
  3. Assign the mlbackend tag to an env variable MOODLE_DOCKER_MLBACKEND_PYTHON_TAG (I guess we could use something like https://docs.docker.com/compose/environment-variables/#the-env-file)
  4. Modify the external services yml file so the image attribute looks like this:
    image: "moodlehq/moodle-mlbackend-python:${MOODLE_DOCKER_MLBACKEND_PYTHON_TAG}"

@stronk7
Copy link
Member

stronk7 commented Jan 9, 2020

I'm not aware of any other image/external service needing such differentiation. If something... optionally, maybe selenium versions... but that's apart/exceptional.

Yes... we should be able to do that via version.php parsing (if the env is not manually set). From the up shell & cmd scripts. And keep a table of branches and versions maintained somehow.

Cannot imagine another way, right now... if anybody knows... please, share your wisdom here!

Ciao :-)

@dmonllao
Copy link
Author

I've coded the approach proposed above in a separate branch: dmonllao@e1afc7f. I've set it as a separate branch because:

  • I have no idea how to code this in bin/moodle-docker-compose.cmd
  • What happens with master needs to be discussed. If we set the MOODLE_DOCKER_MLBACKEND_PYTHON_TAG variable mlbackend_python tests will be skipped silently. If we want you to update moodle-docker every time there is a new major release I would suggest not setting the value for > 38 so that docker composer up errors and it can be updated.

@stronk7
Copy link
Member

stronk7 commented Jan 15, 2020

OT, for the records (spaming all you, beloved moodlers):

Now the python mlbackend server images are being auto-built. More info @ moodlehq/moodle-docker-mlbackend-python#1

Ciao :-)

@scara
Copy link
Contributor

scara commented Jan 15, 2020

Hi @stronk7,

I have no idea how to code this in bin/moodle-docker-compose.cmd
is that under your radar?

I could try to do that but not in the very next days.

HTH,
Matteo

@stronk7
Copy link
Member

stronk7 commented Jan 15, 2020

Yeah, I'll be looking later this week. Need to close it (integration-wise). I bet it's going to be tricky (but haven't had time to look to the login implemented in unix shell yet).

Will share here any progress (or lack of).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants