Build container feature #34
Conversation
…is not absolute and break the ctf deploy command.
…e. Wrote a module.tf parser.
…module.tf once the build is over, reapply to destroy the build container once the file is extracted.
MOBergeron
force-pushed
the
build-container-feature
branch
from
30d37a4 to
c57e30d
Compare
MOBergeron
force-pushed
the
build-container-feature
branch
from
776c939 to
1378d7c
Compare
…properly test the workflow and often crashes.
MOBergeron
force-pushed
the
build-container-feature
branch
from
5cd8e79 to
5fffeea
Compare
| from ctf.utils import find_ctf_root_directory, get_ctf_script_templates_directory | ||
|
|
||
| app = typer.Typer() | ||
|
|
||
|
|
||
| @unique | ||
| class Template(StrEnum): |
There was a problem hiding this comment.
imo ce genre de refactor est inutile et nuisible. C'est pertinent pour des gros projets mais c'est overengineered pour quelque chose de petit comme ici. On a pas besoin de la classe Template dans autre chose que new() et y'a pas d'indication que ça sera le cas dans le futur.
Je comprend que c'est dans un but de séparer la business logic des modèles et autres éléments plus statiques et que ça feele plus clean, mais je trouve que c'est un net négatif considérant que c'est plus compliqué pour comprendre le code qui se trouve ici (force d'aller dans une autre page juste pour voir ce qu'il y a dans l'enum qui est spécifique à cette commande) et qu'il n'y a aucun gain technique ou d'architecture.
Pas besoin de le changer de place, c'est fait, c'est plus un commentaire de vision pour le futur. Je pense que plus on complexifie notre codebase, même par des petits refactors simples comme ça, plus la barrière d'entrée sera haute et plus la charge mentale nécessaire pour comprendre la codebase sera grande.
There was a problem hiding this comment.
Un exemple: si je veux ajouter un template, je dois ajouter une entrée dans models.py, et maintenant je dois me poser la question: "est-ce que Template est utilisé ailleurs? Avant, je n'avais pas à me poser la question, car l'Enum était dans le fichier qui l'utilisait, qui contient une commande, c'est implicite que c'est la seule place où c'est utilisé
ctf/templates/new/common/main.tf.j2
Outdated
| "pool" = "default" | ||
| "path" = "/" | ||
| # This limit should only be adjusted if you NEED more resources. | ||
| "size" = "1GiB" |
There was a problem hiding this comment.
est-ce qu'1go est suffisant pour un container de type build? Jpense que ça pourrait prendre plus de place
There was a problem hiding this comment.
Je me disais que c'est au designer de le modifier. On pourrait mettre un commentaire "change me" avec explication.
There was a problem hiding this comment.
Il faudrait aussi changer le commentaire plus haut qui dit de ne pas toucher à cette section
| @@ -15,3 +15,9 @@ all: | |||
| ansible_incus_project: {{ data.name }} | |||
|
|
|||
| # Add variables if needed here. | |||
| {% if data.with_build %} | |||
| build: | |||
There was a problem hiding this comment.
ajouter un commentaire en lien avec ce que ça représente et ça sert à quoi
| @@ -0,0 +1,60 @@ | |||
| - name: "Build container" | |||
There was a problem hiding this comment.
ajouter un commentaire pour dire à quoi sert ce fichier
| install_recommends: false | ||
| upgrade: full | ||
|
|
||
| # Install tools to compile/build the track |
There was a problem hiding this comment.
changer le commentaire pour mentionner d'ajouter whatever dépendance est nécessaire pour build son challenge ici
| return 0; | ||
| } | ||
|
|
||
| # Compile the program |
There was a problem hiding this comment.
modifier le commentaire pour dire de changer au besoin selon l'outil à utiliser pour build
| exit(code=0) | ||
|
|
||
| for track in distinct_tracks: | ||
| if track.require_build_container: | ||
| run_ansible_playbook( |
There was a problem hiding this comment.
pourrait être pertinent d'ajouter un log statement pour bien dire ce qui se passe?
No description provided.