Skip to content

Robot powrap en GitHub #1786

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

Open
rtobar opened this issue Dec 27, 2021 · 13 comments
Open

Robot powrap en GitHub #1786

rtobar opened this issue Dec 27, 2021 · 13 comments
Labels

Comments

@rtobar
Copy link
Collaborator

rtobar commented Dec 27, 2021

Esto es sólo una idea por el momento, que la quiero dejar flotando: podríamos tener un robot en GH al que uno le pueda pedir en una PR que corra powrap sobre los archivos para así tener que evitar que los usuarios lo tengan que correr de forma manual después de una corrección que se lleva a cabo a través de la UI de los PR.

@cacrespo
Copy link
Collaborator

Me gusta mucho la idea.
+1

@humitos
Copy link
Collaborator

humitos commented Oct 28, 2022

Esto se puede hacer con un GitHub Action.

El problema que le veo a "aplicar cambios en remoto que no están en local" es que luego genera conflictos a la hora de hacer otro push a la rama ya que primero tendría que hacer pull para descargar los cambios del "robot". Así, generando más problemas en el workflow para usuarios sin mucho conocimiento de Git.

Lo que se podría hacer sino, sería que el powrap corra al momento de hacer el merge en la rama 3.11. De esa forma, los traductores no se tienen que preocupar por el powrap ya que se corre automáticamente al momento de hacer merge.

@erickisos
Copy link
Contributor

Me parece una buena opción, en teoría el comment actual serviría para ambos casos, como un "Coméntalo antes de darle merge", pero si les parece mejor hacerlo completamente independiente, hacerlo no sería un problema mayor.
Voy a checar los otros comments también y vuelvo acá, sería bueno conocer la opinión general en esto.

@rtobar
Copy link
Collaborator Author

rtobar commented Oct 30, 2022

Sí, de buenas a primeras también me suena mejor la idea de hacer el powrap automáticamente al momento de hacer el merge para que los usuarios no tuviesen que lidiar con el wrapping. Si bien en ese caso igual se hacen cambios a los archivos de forma automática que pueden pillar desprevenidos a usuarios menos acostumbrados a git, sí es verdad que al momento de hacer el merge probablemente el archivo en cuestión ya no se vuelva a editar de forma local, así que las probabilidades de confusión son mucho menores.

@humitos ¿cómo ves el workflow exactamente? Por lo que veo GH no permite modificar el merge commit mismo, por lo que habría que hacer un commit extra antes en la rama con los cambios que se quieren mergear, o uno después ya en la rama principal. Supongo que la alternative de hacerlo después no es la mejor (la idea es mergear los cambios ya listos, ¿no?), por lo que quedaría la opción de crear el commit antes. Lo que nos lleva de vuelta a cómo usar este GH action que escribió @erickisos. Quizás puede quedar restringido para que sólo los que están revisando un PR puedar usarlo, y así se limita su uso sólo para cuando ya están todo el resto de los cambios listos.

@cmaureir
Copy link
Collaborator

cmaureir commented Nov 1, 2022

Screenshot 2022-11-01 at 18-15-52 traducido archivo c-api_init_config by rodpoblete · Pull Request #2143 · python_python-docs-es

solo para agregar más leña al fuego (de buena manera), todos esos PR que están fallando son por powrap.

@mmmarcos
Copy link
Collaborator

mmmarcos commented Nov 2, 2022

Otra opción podría ser construir una imágen Docker a partir de Ubuntu 22.04, instalar las dependencias y usarlo para ejecutar powrap en local. La imágen se construye una única vez y podría incluso adaptarse en caso de necesitar upgrades. La complejidad de ejecutar la imagen (docker run... y demás) puede esconderse detras de un script (make wrap?).

La única dificultad que veo es que requiere instalar Docker en local, pero esto parece un "precio" razonable a pagar para quienes que no puedan usar un OS reciente. De hecho este workflow podría ser sólo una alternativa para desbloquear este problema.

@cmaureir
Copy link
Collaborator

cmaureir commented Nov 2, 2022

Si la gente tiene problemas con instalar y ejecutar un comando ¿de verdad crees que pedirles que instalen docker y lo ejecuten lo podría solucionar?
Pues yo lo veo al mismo nivel de "mejor que instalen Ubuntu 22.04 en una máquina virtual y lo miren ahí".
Si están en windows, como la mayoría de personas, les va a dar un montón de problemas también, con permisos y todo :(
Docker es un cañon muy grande para matar una mosca tan pequeña.

Incluso, saldría más fácil a la gente de windows que use WSL y que hagan la prueba ahí, el problema, al igual que docker, es que no van a tener configurada su cuenta Git, entonces son más problemas.
IMVHO veo más sencillo, o que gente lo pueda ir haciendo a quienes tengan problema, o enseñarles a que lo solucionen con poedit.

@humitos
Copy link
Collaborator

humitos commented Nov 3, 2022

Adiero a lo que dice @cmaureir. Lo idea es no complicarle más la vida a los colaboradores.

powrap no es necesario para hacer el build. Simplemente, "es una norma de estilo" que se decidió hace un tiempo y se automatizó porque era sencillo hacerlo. Ahora las cosas cambiaron, y powrap está dando más problemas que beneficios. Por lo tanto, yo lo quitaría del build checker de momento para que las PRs validen y los colaboradores no se confundan. Además, de poder hacer merge de las PR --y buscaría otra solución mientras tanto.

Esa otra solución, tranquilamente podría ser "no automática" de momento y que uno de los admins corramos powrap una vez por semana o similar sobre la rama 3.11 (incluso esto mismo puede ser un GHA que corra con un cron). Creo que nos estamos complicando mucho con esto y mientras tanto bloqueando/confundiendo más a los traductores 😄

@mmmarcos
Copy link
Collaborator

mmmarcos commented Nov 3, 2022

Me convencieron 😆 @cmaureir @humitos
+1 por sacarlo del build checker

@rtobar
Copy link
Collaborator Author

rtobar commented Nov 4, 2022

Yo también estoy de acuerdo la verdad con @humitos, lo más fácil es que los usuarios simplemente no tengan que preocuparse de este detalle.

@erickisos
Copy link
Contributor

Por lo poco que veo podemos trasladar este action a ambos casos, que quienes se sientan cómodos lo ejecuten directamente en su PR, y para quienes sea indiferente, lo puedan dejar sin inconvenientes, creo que podemos poner un segundo evento para que en cuanto la rama principal (3.11) reciba un merge, ejecutemos el action solo para verificar esas reglas de estilo.
El único detalle que encontraría, es que generaría un commit extra para todos esos PRs que no lo ejecutaron, pero no considero eso un problema mayor.

@cmaureir
Copy link
Collaborator

cmaureir commented Nov 5, 2022

Igual a mi lo de hacer powrap como parte del merge me parece super idea. Es raro que alguien venga a una rama antigua a hacer algunos cambios y de ser así, por lo general la gente tendrá que aprender a actualizar el estado de la rama primero.

El único problema que le veo a eso (que prefiero antes de estar haciendo powrap de vez en cuando) es que tenemos que hacer una revisión buena, porque si nos encontramos con el caso de un archivo mal formateado, powrap va a explotar. Así que parece que lo suyo sería dejar el powrap check, pero que no afecte el fallo del CI, para que quien mire el build, sepa si hay algún error de sintaxis o algo (aunque eso de seguro se transforma en un error al construir la doc)

@github-actions
Copy link

Este issue lleva un tiempo sin actualizaciones. ¿Estás trabajando todavía?\nSi necesitas ayuda 🆘 no dudes en contactarnos en nuestro grupo de Telegram.

@github-actions github-actions bot added the stale label Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants