Produits Entr'ouvert » Passerelle

Peut-être factorisé le code entre les deux et mettre ce qu'il faut dans passerelle.utils.

J'imposerais bien le "dir", parce qu'en fait il faut éviter de créer des "tmpxxx" dans le répertoire de travail (où un client FTP va venir chercher ses choses). Genre :

def atomic_write(filepath, tmpdir):
    '''Return a file descriptor to a temporary file using NamedTemporaryFile
       which will be atomically renamed to filepath if possible.

       Atomic renaming is only possible on the same filesystem, so the
       tmpdir must be in the same filesystem as filepath.
    '''
    fd = tempfile.NamedTemporaryFile(dir=tmpdir, delete=False)
    try:
       ...

Thomas Noël a écrit :

J'imposerais bien le "dir", parce qu'en fait il faut éviter de créer des "tmpxxx" dans le répertoire de travail (où un client FTP va venir chercher ses choses). Genre :

[...]

Dans ce cas j'imposerai bien un:

   import errno

   tmpdir = kwargs.get('dir')
   if not tmpdir:
       tmpdir = os.path.join(settings.MEDIA_DIR, 'tmp')
       if not os.path.exists(tmpdir):
           try:
               os.makedirs(tmpdir)
           except OSError as e:
               if e.errno != errno.EEXIST:
                   raise

dans l'idée que ce soit le plus simple à réutiliser possible (et si un jour on ne met pas tout /var/lib/passerelle/<domain>/media/ sur la même partoche, on se posera alors la question de faire autrement).

Et celui qui créera un connecteur nommé 'tmp' qui stocke des fichiers sera châtié.

Pourquoi pas un .tmp?
Cela nous évitera à penser au chatiement.

Allons-y pour un classique et sans surprise "tmp" (je laisse l'attribution à Serghei mais je ne sais pas si Benjamin va s'en occuper (mini-congés))

Benjamin va relire :)

Heu... Relire quoi ?... Au pire je peux (re)faire son patch, si tu es pressé et pas trop dispo

Thomas Noël a écrit :

Discuté avec Serghei : on attend de savoir si les clients cityweb peuvent consommer du XML non-zipé pour savoir s'il y aura quelque chose à factoriser. (J'attribue à Serghei qui a demandé aux clients concernés)

C'est quoi le rapport ? Zip ou XML il faut les créer atomiquement.

Quel interet d'écrire atomiquement un XML puis le ZIP dans lequel on le place?

Pour moi, on ajoute la fonction atomic_write et on écrit des XML avec cette fonction.

Je me dis qu'on devrait utiliser le atomic_write au niveau du save du default_storage.
Ainsi toutes les ecritures de fichiers dans Publik gagnent l'atomicité.

Euh..

    target_dir = kwargs.get('dir') or os.path.dirname(filepath)
    if not os.path.exists(target_dir):
        os.makedirs(target_dir)

C'est le merge de Frankenstein.

Benjamin Dauvergne a écrit :

Euh..

[...]

C'est le merge de Frankenstein.

Pas compris que tu ne reprenais pas tout mon code.

Aussi le deuxième patch n'a pas de rapport avec ce ticket...

Benjamin Dauvergne a écrit :

Pas compris que tu ne reprenais pas tout mon code.

J'ai appliqué ton patch en local: il y a 2 fichiers. Ou je rate un truc?

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

Pas compris que tu ne reprenais pas tout mon code.

J'ai appliqué ton patch en local: il y a 2 fichiers. Ou je rate un truc?

Non mais j'avais mis en commentaire une modification du code (mais pas testé, d'où l'erreur MEDIA_DIR/MEDIA_ROOT) pour répondre au commentaire de Thomas, j'espérais que tu l'intégrerais, je l'ai fait dans le dernier patch que j'ai soumis.

Dans 0001, tu ne vérifies pas que le(s) répértoire(s) du fichier cible existe (c'est ce que fait default_storage.save).

Donc la fonction créera le répértoire temporaire mais pas celui ou le fichier doit être ensuite deplacé.

IMO, la fonction atomic_write doit faire:

    # create target file directory if it doesn't exist
    dirname = os.path.dirname(filepath)
    if not os.path.exists(dirname):
        os.makedirs(dirname)

Serghei Mihai a écrit :

Dans 0001, tu ne vérifies pas que le(s) répértoire(s) du fichier cible existe (c'est ce que fait default_storage.save).

Ce n'est pas le job d'atomic_write, de toute façon comme vu dans actesweb les modules souhaitent des autorisations spécifiques, si on déporte la création du répertoire cela va compliquer les choses sur ce point.

Donc la fonction créera le répértoire temporaire mais pas celui ou le fichier doit être ensuite deplacé.

Ce n'est pas grave, c'est le boulot de l'appelant de savoir où il veut écrire et comment (le but c'est juste d'améliorer la robustesse d'un simple with open(..., 'w') as fd: fd.write(...).

Par contre je me rends compte que vu comment actesweb est déployé à Dreux je dois absolument avoir le répertoire 'tmp' dans le répertoire de destination (actesweb est un lien vers /srv/nfs/chroot/ville-de-dreux/home/ville-de-dreux/actesweb et donc le tmp dans "media" ne marchera pas).

Je trouve un peu dommage ce fonctionnement (de même que les masques de droit qui sont dans le code n'ont rien à faire ici, c'est au déploiement qu'on doit faire des liens vers /srv et mettre les droits d'écriture pour le groupe sur les répertoires et pas sur les fichiers, normalement FILE_UPLOAD_PERMISSIONS doit suffire à s'assurer que les groupes ont les droits en lecture).

Benjamin Dauvergne a écrit :

Serghei Mihai a écrit :

Dans 0001, tu ne vérifies pas que le(s) répértoire(s) du fichier cible existe (c'est ce que fait default_storage.save).

Ce n'est pas le job d'atomic_write, de toute façon comme vu dans actesweb les modules souhaitent des autorisations spécifiques, si on déporte la création du répertoire cela va compliquer les choses sur ce point.

Ok, donc chaque connecteur devra s'assurer d'avoir son répértoire avant d'appeler atomic_write. Je vais adapter mon patch dans #32091.

Par contre je me rends compte que vu comment actesweb est déployé à Dreux je dois absolument avoir le répertoire 'tmp' dans le répertoire de destination (actesweb est un lien vers /srv/nfs/chroot/ville-de-dreux/home/ville-de-dreux/actesweb et donc le tmp dans "media" ne marchera pas).

C'est fait pour donner accès ssh (pour faire SFTP ou rsync) aux clients. Depuis leur $HOME on fait un lien symbolique vers le répértoire du connecteur.
C'est le cas pour actesweb mais aussi pour Cityweb et MDEL. Il faudrait alors revoir notre façon de faire.

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

Serghei Mihai a écrit :

Dans 0001, tu ne vérifies pas que le(s) répértoire(s) du fichier cible existe (c'est ce que fait default_storage.save).

Ce n'est pas le job d'atomic_write, de toute façon comme vu dans actesweb les modules souhaitent des autorisations spécifiques, si on déporte la création du répertoire cela va compliquer les choses sur ce point.

Ok, donc chaque connecteur devra s'assurer d'avoir son répértoire avant d'appeler atomic_write. Je vais adapter mon patch dans #32091.

Oui comme avant quoi, sinon on est parti pour réécrire FileStorage.

Par contre je me rends compte que vu comment actesweb est déployé à Dreux je dois absolument avoir le répertoire 'tmp' dans le répertoire de destination (actesweb est un lien vers /srv/nfs/chroot/ville-de-dreux/home/ville-de-dreux/actesweb et donc le tmp dans "media" ne marchera pas).

C'est fait pour donner accès ssh (pour faire SFTP ou rsync) aux clients. Depuis leur $HOME on fait un lien symbolique vers le répertoire du connecteur.

C'est l'inverse qui est fait il y a un lien symbolique du répertoire du connecteur vers le sftp.

C'est le cas pour actesweb mais aussi pour Cityweb et MDEL. Il faudrait alors revoir notre façon de faire.

Le fait est que ces histoires n'ont rien à faire dans le code de passerelle et pour l'instant y sont, un chmod + mount -bind au niveau du serveur sftp aurait suffit.

Benjamin Dauvergne a écrit :

C'est l'inverse qui est fait il y a un lien symbolique du répertoire du connecteur vers le sftp.

Alors on peut faire le atomic_write sans soucis.

Si pour ActesWeb on a fait n'importe quoi on réparera dans un ticket à part.

Frileux, je suis d'avis de pousser uniquement la fonction, puis l'usage de celle-ci dans cityweb (#32091).

Ok mais faut quand même me valider le ticket :)