Fix issue for building docker container #91

luclaurent · 2024-11-29T14:44:51Z

Many improvements and updates on

workflow (update versions and missing id
Dockerfile to use new setup version and new testing

…ate Dockerfile

…or building container

luclaurent · 2024-11-29T15:01:32Z

Note that ths proposition will not work directly due to missing credentials for ghcr. You must update them @nennigb.

nennigb · 2024-11-29T15:07:39Z

.github/workflows/build-container.yml

@@ -13,24 +13,25 @@ jobs:
    name: 'Build amc2moodle container'
    runs-on: ubuntu-latest
    steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v4


I suggest @main instead of @v4 for all actions.
Since the version doesn't matter, @main allow to use always the last version.

Caution @main is not always available (check for @master)

nennigb · 2024-11-29T15:09:54Z

.vscode/settings.json

+        "-p",
+        "test_*.py"
+    ],
+    "python.testing.pytestEnabled": false,


Is this file really required ?

probably not with the future new version using pyproject.toml (vscode will read it to identify tests)

I removed it in future version see #90

nennigb · 2024-11-29T15:28:01Z

amc2moodle/utils/misc.py

+        return cls
+    return decorate
+
+def decorator_set_cwd(directory):


Perhaps there are more pythonic way to do this ?
Defined in unittest famework ?

I don't know but it works well :-)

nennigb · 2024-12-03T20:08:12Z

docker/Dockerfile

+    python3-venv \
+    git && \
+    apt clean &&\
+    rm -rf /var/lib/{apt,dpkg,cache,log}/



better to use apt-get (to avoid WARNING: apt does not have a stable CLI interface. Use with caution in scripts.)

nennigb · 2024-12-03T20:12:20Z

docker/Dockerfile

-RUN pip3 install -U pip && \
-    pip3 install .
+RUN mkdir -p ${VENVDIR} && \
+    python -m venv ${VENVDIR}


Why installing the package in a venv ?
No problem with that, I am just curious ;-)
Doesn't seems to create problem.

in order to avoid "error: externally-managed-environment" when use the system python/pip

nennigb · 2024-12-04T07:58:42Z

docker/Dockerfile

+    python3-venv \
+    git && \
+    apt clean &&\
+    rm -rf /var/lib/{apt,dpkg,cache,log}/

 # move policy file
 RUN mv /etc/ImageMagick-6/policy.xml /etc/ImageMagick-6/policy.xml.off


On my tests, docker install imageMagick 7 thus the path is wrong.
In interactive session it seems to work without this line. The default policy has changed in v7, perhaps we could just comment this line.
To see the policy in v7

magick identify -list policy

https://imagemagick.org/script/security-policy.php

Normally it should be catch in the unittests with the tinymonk.pdf file.

nennigb · 2024-12-04T08:02:48Z

docker/Dockerfile

-    python -m amc2moodle.moodle2amc.test
-
-
+RUN python -m pytest --pyargs amc2moodle


CI will fail until

[project.optional-dependencies] test = ["pytest", "pytest-cov[all]"]

will be merged into the master branch used for docker build.
We could merge by the way.

nennigb · 2024-12-04T08:22:01Z

pyproject.toml

@@ -26,6 +26,10 @@ dependencies = [
 ]
 requires-python='>=3.8'

+[project.optional-dependencies]
+test = ["pytest", "pytest-cov[all]"]
+lint = [ "black", "flake8"]


By default
https://hatch.pypa.io/1.13/config/internal/static-analysis/
hatch use ruff: https://github.com/astral-sh/ruff, 99.9% compatible with black.
Not sure we need this line: lint = [ "black", "flake8"]
Perhaps we could also configure ruff or have a discussion on rules

hatch default is line 120, which looks good for me. It could be put in the pyproject.toml to avoid changes

[tool.ruff] # Allow lines to be as long as 120. line-length = 120

I prefer simple quotes which is the present convention and looks less bulky.

[tool.ruff.format] quote-style = "single"

If we choose ruff, we need to modify the readme.

nennigb · 2024-12-04T20:59:13Z

Another thing to fix (one day) would be the log that grows indefinitely if the docker is not stopped.
In my opinion it is not a big issue since it not the heart of the project.

@main

* update pyproject.toml to integrate the use of ruff as code analyser * update Docker's files (Dockerfile, README, bash script) to ensure logrotation and a few fixes * many improvments of typos * update workflow to use @main on actions

luclaurent · 2024-12-12T12:17:26Z

Another thing to fix (one day) would be the log that grows indefinitely if the docker is not stopped. In my opinion it is not a big issue since it not the heart of the project.

I integrate a new management of the logs for amc2moodle and moodle2amc. I use logrotate with a maximum size of the log of 5MB, keeping only 3 old logs and run logrotate every 60s. Note that the --silent option has been removed in the bash script to allow logging on the daemon log.

nennigb

For me it goods. You can have a look on the detailed the comments.
I let you merge when you are ready.

nennigb · 2024-12-13T14:37:38Z

docker/README.md

@@ -15,19 +15,28 @@ The container proposes to mount two volumes:
    - `/tmp/work` (**mandatory mount**) that must be link to a specific folder on the host computer
    - `/tmp/daemon` (optional mount) that store the log file of the daemon

-For instance, the container can be run in CLI (in detached mode) with the following command:
+For instance, the container can be run in CLI (in detached mode) with the following command (*by relacing `DIRA` and if necessary `DIRB`):


typo "replacing"

nennigb · 2024-12-13T14:38:23Z

docker/README.md

+In case of permission issue to mount folder(s), consider adding option `--user "$(id -u):$(id -g)"` such as the running command is:
+
+```
+docker run --name amc2moodle --user "$(id -u):$(id -g)" -d -v "DIRA:/tmp/work" -v "DIRB:/tmp/daemon" ghcr.io/nennigb/amc2moodle


nennigb · 2024-12-13T15:43:12Z

docker/autorun-amc2moodle.sh


 #log manage
 now=$(date +"%Y-%m-%d_%H-%M-%s")
 mkdir -p ${LOG_DIR}
 LOGFILE=${LOG_DIR}/${now}_autorun.log
+init_logrotate
+run_logrotate &

 {


Seems goods. I run test and log looks good. The rotation also works well.

nennigb · 2024-12-13T17:03:12Z

Just a question the modification with f strings were automatic or manual ?
I checked most of them but just in case.

luclaurent · 2024-12-13T20:36:30Z

Just a question the modification with f strings were automatic or manual ? I checked most of them but just in case.

I ran hatch fmt by mistake. The correct syntax to only check is --check. But after that, I kept the changes ...

luclaurent added 6 commits November 29, 2024 15:41

try update docker building as github actions

cd8f921

swicth workflo to ran on any cases

e19013e

add missing id in build-container.yml

58b8c44

update pyproject with optional dependencies for testing and start upd…

8e7a728

…ate Dockerfile

move covergae optional dependencies to test

4928c5d

fix typos in Dockerfile + reactivate version activation of workflow f…

e143a88

…or building container

luclaurent linked an issue Nov 29, 2024 that may be closed by this pull request

Fix issue for building docker container #89

Closed

nennigb reviewed Nov 29, 2024

View reviewed changes

update all versions of actions to @main

ee8790b

luclaurent assigned luclaurent and nennigb Dec 2, 2024

luclaurent added 2 commits December 3, 2024 10:34

Merge branch 'master' into fix-issue-for-building-docker-container

64449ef

try to deal with unstable latex repo

5146354

nennigb requested changes Dec 4, 2024

View reviewed changes

Many changes

512fa65

* update pyproject.toml to integrate the use of ruff as code analyser * update Docker's files (Dockerfile, README, bash script) to ensure logrotation and a few fixes * many improvments of typos * update workflow to use @main on actions

nennigb approved these changes Dec 13, 2024

View reviewed changes

luclaurent merged commit fa5d577 into master Dec 13, 2024
24 checks passed

luclaurent deleted the fix-issue-for-building-docker-container branch December 13, 2024 20:36

nennigb mentioned this pull request Dec 14, 2024

Fix deprecation warning #95

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue for building docker container #91

Fix issue for building docker container #91

luclaurent commented Nov 29, 2024

luclaurent commented Nov 29, 2024

nennigb Nov 29, 2024

luclaurent Dec 13, 2024

nennigb Nov 29, 2024

luclaurent Nov 29, 2024 •

edited

Loading

luclaurent Nov 29, 2024 •

edited

Loading

nennigb Nov 29, 2024

luclaurent Nov 29, 2024

nennigb Dec 3, 2024

luclaurent Dec 10, 2024

nennigb Dec 3, 2024

luclaurent Dec 10, 2024

nennigb Dec 11, 2024

nennigb Dec 4, 2024 •

edited

Loading

luclaurent Dec 10, 2024

nennigb Dec 4, 2024

nennigb Dec 4, 2024

luclaurent Dec 12, 2024

nennigb commented Dec 4, 2024

luclaurent commented Dec 12, 2024 •

edited

Loading

nennigb left a comment

nennigb Dec 13, 2024

nennigb Dec 13, 2024

nennigb Dec 13, 2024

nennigb commented Dec 13, 2024

luclaurent commented Dec 13, 2024

		python -m amc2moodle.moodle2amc.test


		RUN python -m pytest --pyargs amc2moodle

Fix issue for building docker container #91

Fix issue for building docker container #91

Conversation

luclaurent commented Nov 29, 2024

luclaurent commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luclaurent Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

luclaurent Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nennigb Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nennigb commented Dec 4, 2024

luclaurent commented Dec 12, 2024 • edited Loading

nennigb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nennigb commented Dec 13, 2024

luclaurent commented Dec 13, 2024

luclaurent Nov 29, 2024 •

edited

Loading

luclaurent Nov 29, 2024 •

edited

Loading

nennigb Dec 4, 2024 •

edited

Loading

luclaurent commented Dec 12, 2024 •

edited

Loading