search

LuisArostegui / MyWallet

GNU General Public License v3.0
0 stars 3 forks source link

[IV-21-22] Objetivo 5 #35

Open LuisArostegui opened 2 years ago

LuisArostegui commented 2 years ago

Observación para este objetivo

Se recuerda al estudiante que este objetivo no consiste en “poner las tres o cuatro imágenes oficiales del lenguaje que esté usando y quedarme con Alpine”. Para superarlo, tiene realmente que demostrar que ha seguido las mejores prácticas en la búsqueda y elección de un contenedor base que sea el más adecuado para testear el repositorio.

Se termina optando por Alpine, pero se analizan, basandonos en varios requisitos, varias imagenes hasta obtener la mejor elección posible para nuestro proyecto.

Otros cambios

Se ha desarrollado #28 para avanzar con la lógica de negocio del proyecto. Se cambia de framework de tests. Se documenta y se informa en #36

JJ commented 2 years ago

⛹ Revisores → @Olasergiolas @joaquingv12 @amerigal

LuisArostegui commented 2 years ago

En general lo veo bastante completo, bien trabajado el uso de buenas prácticas y la documentación, pero creo que sería conveniente completar la justificación de la elección de la imagen base para el contenedor.

Por otro lado, pienso que sería apropiado que el milestone reflejara de manera más explícita el trabajo realizado en este objetivo.

También incluyo una observación sobre la automatización de la subida del contenedor.

Un último comentario menor es que los enlaces del comentario de este PR en la lista de comprobación no se están renderizando bien porque hay un espacio entre los corchetes y los paréntesis.

Gracias por todos los comentarios. He actualizado todo lo que me has comentado. Modifique también el milestone para indicar el trabajo que se realiza en el objetivo.

amerigal commented 2 years ago

Gracias por todos los comentarios. He actualizado todo lo que me has comentado. Modifique también el milestone para indicar el trabajo que se realiza en el objetivo.

No hay de qué! Diría que ahora está más clara y mejor argumentada la decisión de la imagen base :star: Te he comentado sobre una cuestión que creo que sería necesaria perfilar del despliegue de la imagen y que también influiría en la documentación y descripción del milestone, pero efectivamente la descripción del milestone ya está más completa con el trabajo de este objetivo.

LuisArostegui commented 2 years ago

@JJ, listo para revisión. Pese a que dos compañeros no han podido hacer la revisión pienso que con las correciones realizadas por @amerigal está bastante correcto el objetivo. Disculpe las molestias si no lo está bajo su punto de vista.

LuisArostegui commented 2 years ago

He actualizado la configuración para la publicación de la imagen mientras estaba se estaba trabajando en el objetivo 6 para avanzar, de ahi que no se ejecute correctamente los tests en Circle CI, ya que no hay configuración en esta rama, pero si se ejecuta correctamente la publicación de la imagen que es lo relativo a este objetivo.

lentes4k commented 2 years ago

Vengo del hito 6 (es como venir del futuro... jajajaja) a comentarte lo de seguir las buenas prácticas y eliminar los comentarios inline (puedes dejar aclaraciones o cosas que sean 100% necesarias) lo demás debería de verse reflejado en la documentación

LuisArostegui commented 2 years ago

Vengo del hito 6 (es como venir del futuro... jajajaja) a comentarte lo de seguir las buenas prácticas y eliminar los comentarios inline (puedes dejar aclaraciones o cosas que sean 100% necesarias) lo demás debería de verse reflejado en la documentación

Perfecto, lo acabo de corregir. He dejado sin comentarios el Dockerfile y el fichero dockerhub.yml. Todos los comentarios que tenía los he puesto en la documentación. Aparte he agregado una pequeña sección comparando el tamaño de las distintas imagenes base para comparar las imagenes de Debian con la de Alpine que creo que puede ser interesante hacer un estudio más profundo de todas las imagenes que podemos usar. Muchas gracias por el comentario. Ahora te agradezco el comentario del Objetivo 6 jajajajajajajaja

LuisArostegui commented 2 years ago

Una duda que tengo... Tengo claro que al hacer push y al modificarse el fichero Dockerfile o go.mod se va a lanzar la publicación de una nueva imagen del proyecto, es lo que indiqué en el fichero dockerhub.yml pero claro al actualizar el PR y como se ha modificado el fichero Dockerfile también se ha ejecutado la publicación de la imagen... Es del todo correcto? Debería de dejar solamente que se generé una nueva imagen cuando se actualice el PR y quitar la sección de cuando se hace push? Quité la parte de que si hago push a la rama main se generé una nueva imagen ya que nunca vamos a hacer un push al main porque en la asignatura se avanza haciendo PR...

Gracias de antemano a todos.

JJ commented 2 years ago

Una duda que tengo... Tengo claro que al hacer push y al modificarse el fichero Dockerfile o go.mod se va a lanzar la publicación de una nueva imagen del proyecto, es lo que indiqué en el fichero dockerhub.yml pero claro al actualizar el PR y como se ha modificado el fichero Dockerfile también se ha ejecutado la publicación de la imagen... Es del todo correcto? Debería de dejar solamente que se generé una nueva imagen cuando se actualice el PR y quitar la sección de cuando se hace push? Quité la parte de que si hago push a la rama main se generé una nueva imagen ya que nunca vamos a hacer un push al main porque en la asignatura se avanza haciendo PR...

Gracias de antemano a todos.

La imagen actualizada debería funcionar también en esta rama. En todo caso, tienes un error en Circle, tendrás que solucionarlo...

LuisArostegui commented 2 years ago

es porque he estado trabajando en el objetivo 6 mientras estaba con el 5 arreglando algunas cosas. Una solución sería añadir la configuración de Circle CI en esta rama para que no no aparezca el error, pero toda la documentación la mantendría en el objetivo 6 y ya usted lo corrige todo de ahi. Gracias.

JJ commented 2 years ago

es porque he estado trabajando en el objetivo 6 mientras estaba con el 5 arreglando algunas cosas. Una solución sería añadir la configuración de Circle CI en esta rama para que no no aparezca el error, pero toda la documentación la mantendría en el objetivo 6 y ya usted lo corrige todo de ahi. Gracias.

Lo que sea, pero no puede haber tests que no pasen.

LuisArostegui commented 2 years ago

Corregido @JJ . Se comenta todo en #41 .

JJ commented 2 years ago

  • [x] ¿Se han seguido las buenas prácticas en las órdenes que construyen la imagen? ¿Especialmente las relativas a usuarios no privilegiados y demás? Sí, se puede comprobar aquí.

    • [x] ¿La imagen incluye sólo y exclusivamente la infraestructura necesaria para pasar los tests? Sí, se puede comprobar aquí.

    • [x] ¿Se han documentado y enlazado los commits a las imágenes que se han testeado? Sí, se puede comprobar aquí.

    • [x] ¿Está establecido correctamente como WORKDIR el que se va a usar en los tests que se van a lanzar? Si, tanto en el Dockerfile como en la documentación se puede comprobar.

    • [x] ¿Se han establecido claramente los criterios de búsqueda y de aceptación de la imagen base? Sí, se puede comprobar aquí.

    • [x] ¿Tienes claro cuales son las condiciones en las que tiene que actualizarse la imagen en DockerHub y has configurado el workflow de acuerdo con ello? Si, se indica en el fichero dockerhub.yml. Y se indica en la documentación un enlace donde se ha obtenido la información.

Como te he comentado, sólo se están creando cuando cambia main. Ni siquiera tendría que funcionar en esta rama que has creado.

JJ commented 2 years ago

  • [x] ¿Se han seguido las buenas prácticas en las órdenes que construyen la imagen? ¿Especialmente las relativas a usuarios no privilegiados y demás? Sí, se puede comprobar aquí.

    • [x] ¿La imagen incluye sólo y exclusivamente la infraestructura necesaria para pasar los tests? Sí, se puede comprobar aquí.

Es un documento, no un commit donde se hayan hecho pruebas sobre una imagen.

LuisArostegui commented 2 years ago

En todos los objetivos de esta asignatura lo principal es que se entiendan (y se expliquen) los criterios para elegir una u otra herramienta, hasta el punto que se han dedicado varias clases exclusivamente a eso. No lo haces aquí y por eso no superas el objetivo, lo siento.

OK, lo entiendo. Estoy haciendo cambios en la documentación para entregar correctamente los siguientes objetivos. Aun asi le dejo unas dudas por aqui. No me vendría mal alguna tutoría (ya sea en presencial o preguntandole por Telegram) acerca de la documentación, pensaba que mi workflow era mejorable pero en ningún caso la documentación. Voy a investigar más y a repasar conceptos. Gracias.

JJ commented 2 years ago

El sáb, 8 ene 2022 a las 20:45, Luis Arostegui Ruiz (< @.***>) escribió:

En todos los objetivos de esta asignatura lo principal es que se entiendan (y se expliquen) los criterios para elegir una u otra herramienta, hasta el punto que se han dedicado varias clases exclusivamente a eso. No lo haces aquí y por eso no superas el objetivo, lo siento.

OK, lo entiendo. Estoy haciendo cambios en la documentación para entregar correctamente los siguientes objetivos. Aun asi le dejo unas dudas por aqui. No me vendría mal alguna tutoría (ya sea en presencial o preguntandole por Telegram) acerca de la documentación, pensaba que mi workflow era mejorable pero en ningún caso la documentación. Voy a investigar más y a repasar conceptos. Gracias.

Pon las dudas que tengas por aquí o, mejor, por el grupo de Telegram.

LuisArostegui commented 2 years ago

El sáb, 8 ene 2022 a las 20:45, Luis Arostegui Ruiz (< @.**>) escribió: En todos* los objetivos de esta asignatura lo principal es que se entiendan (y se expliquen) los criterios para elegir una u otra herramienta, hasta el punto que se han dedicado varias clases exclusivamente a eso. No lo haces aquí y por eso no superas el objetivo, lo siento. OK, lo entiendo. Estoy haciendo cambios en la documentación para entregar correctamente los siguientes objetivos. Aun asi le dejo unas dudas por aqui. No me vendría mal alguna tutoría (ya sea en presencial o preguntandole por Telegram) acerca de la documentación, pensaba que mi workflow era mejorable pero en ningún caso la documentación. Voy a investigar más y a repasar conceptos. Gracias. Pon las dudas que tengas por aquí o, mejor, por el grupo de Telegram.

Se han corregido bastantes cosas. Tanto documentación como la configuración de Docker y la publicación de la imagen. Avanzaré en el resto de objetivos. El 6 está a pique de revisión en cuanto miré un par de cosas más. Gracias por las respuestas a las dudas.

JJ commented 2 years ago

⛹ Revisores → @amerigal @modejota @Parka015

XileonXL commented 2 years ago

Buenas, no he sido asignado a este objetivo pero paso por aquí para checkearlo y ayudar en lo que pueda :smile:. Te dejo lo que he podido encontrar a mejorar aquí debajo.

Antes de nada, felicitarte por el curro que te has pegado (sobre todo en la parte de testeo de las imágenes).

XileonXL commented 2 years ago
  • golang:<version>-alpine, esta imagen se basa en el proyecto Alpine Linux. Las imagenes Alpine Linux son mucho más livianas que la mayoría de imágenes base de distribución (~5 MB). Esta variante es experimental y no es oficialmente compatible con el proyecto Go. La principal advertencia a tener en cuenta es que utiliza musl libc en lugar de glibc, puede llegar a provocar un comportamiento inesperador en nuestra aplicación. En este artículo se conversa acerca de los problemas que puede traer este tipos de imagenes.

No me deja referenciarla directamente desde los "files changed" porque imagino que se ha mergeado desde el "futuro" como decía el compañero más arriba.

Creo que es conveniente añadir o por lo menos dejar constancia de si esa diferencia de librerías puede afectar a tu proyecto o no. Solo mencionas que puede llegar a provocar un comportamiento inesperado. ¿Lo sabes con certeza o es algo que intuyes? Sería mejor concretar.

gomares commented 2 years ago

Hola buenas! Me he leído tu objetivo y ya que no tienes muchas revisiones pues al menos voy a dejarte la mía Luis 😃 .

LuisArostegui commented 2 years ago
  • golang:<version>-alpine, esta imagen se basa en el proyecto Alpine Linux. Las imagenes Alpine Linux son mucho más livianas que la mayoría de imágenes base de distribución (~5 MB). Esta variante es experimental y no es oficialmente compatible con el proyecto Go. La principal advertencia a tener en cuenta es que utiliza musl libc en lugar de glibc, puede llegar a provocar un comportamiento inesperador en nuestra aplicación. En este artículo se conversa acerca de los problemas que puede traer este tipos de imagenes.

No me deja referenciarla directamente desde los "files changed" porque imagino que se ha mergeado desde el "futuro" como decía el compañero más arriba.

Creo que es conveniente añadir o por lo menos dejar constancia de si esa diferencia de librerías puede afectar a tu proyecto o no. Solo mencionas que puede llegar a provocar un comportamiento inesperado. ¿Lo sabes con certeza o es algo que intuyes? Sería mejor concretar.

Muchas gracias por el comentario. He añadido información sobre los errores que puede acarrear ambas librerías y justificando porque no obtendremos errores si usamos una u otra librería. Espero que asi quede más claro. Gracias 😄 .

LuisArostegui commented 2 years ago

Hola buenas! Me he leído tu objetivo y ya que no tienes muchas revisiones pues al menos voy a dejarte la mía Luis 😃 .

Muchas gracias por los comentarios. Ya he puesto todas las tildes jajajajajajaja. Te dejo un comentario acerca de la caché cuando construyo cada imagen.

LuisArostegui commented 2 years ago

@JJ, listo para revisión.