Skip to content
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

fix(git): branches and pull-request #94

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(git): branches and pull-request #94

wants to merge 2 commits into from

Conversation

bunzli
Copy link
Member

@bunzli bunzli commented Mar 25, 2019

Se cambia la sección de Branches y PR en donde queda interpretable que podemos hacer commits directamente en master.

Copy link
Contributor

@llekn llekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¿Y qué hacemos con los chore(): bump version que hacemos para las app móviles?

@bunzli
Copy link
Member Author

bunzli commented Mar 26, 2019

@llekn cómo funcionan esos commits?

@llekn
Copy link
Contributor

llekn commented Mar 27, 2019

@bunzli Mas o menos así:

  • Tienes uno o varios PRs
  • Se aprueban y mergean
  • Quieres sacar a producción una nueva versión con los cambios... Pero para eso tienes que actualizar la versión de la app para publicarlas en los Stores.
  • Entonces haces esa actualización del número de versión en package.json, package-lock.json y config.xml, lo reflejas en un commit y lo subes directamente a master
  • Luego mergeas master a production y se publican los nuevos cambios.

Igual es una mecánica un poco apestosa, manual y propensa a errores. Feliz si encontramos una mejor manera de lograr lo mismo (tags, version bump en el pipeline de CI, etc)

@bunzli
Copy link
Member Author

bunzli commented Mar 27, 2019

@llekn y por qué no se hace esto con un PR?

@llekn
Copy link
Contributor

llekn commented Mar 27, 2019

Yo creo que principalmente porque es un poco engorroso, pero mi argumento es débil porque el PR está como a 2 clicks de distancia de sólo el commit (y una revisión, quizás eso es lo más doloroso pero podría obviarse en este caso).

Si no nos ponemos nazis con la revisión del PR en este caso, no veo por qué no hacerlo como propone usted, señor @bunzli .

Copy link
Member

@aamatte aamatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Es medio latero hacer PRs de ese caso, pero en realidad sí se podrían hacer para ser consistentes. Así que aprobado 🏅

@llekn
Copy link
Contributor

llekn commented Mar 27, 2019

Aprobado considerando la posibilidad de "self-merge"?

@bunzli
Copy link
Member Author

bunzli commented Mar 27, 2019 via email

@bunzli
Copy link
Member Author

bunzli commented Mar 27, 2019

@aamatte @llekn agregué un párrafo para los feature branches

@difernandez
Copy link
Contributor

@bunzli una duda con lo de feature branch: cuando se haca el gran merge a master habría que pedir code review de nuevo no? Entonces me revisarían ese código dos veces?

Y otra cosa, quizás en esta sección (o en otra no se), se podría agregar algo del objetivo de froggo, lo de repartir los PRs

@bunzli
Copy link
Member Author

bunzli commented Mar 27, 2019

Muy buenos puntos.

Creo que va a quedar medio duplicado el review, pero seguramente el PR va a ser bien grande y va a tender a una revisión del estilo LGTM.

Tengo pensado agregar algo de froggo (lo anoté como pega de viernes) en la guía.

@blackjid blackjid self-requested a review June 14, 2019 16:09
Copy link
Contributor

@difernandez difernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull reminder me recuerda que nunca aprobé esto explícitamente 😅

@blackjid
Copy link
Member

@bunzli quieres arreglar esos conflictos?

@bunzli bunzli requested a review from gmq March 8, 2021 23:38
Copy link
Member

@gmq gmq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tal vez sería bueno agregar que recomendamos borrar el branch una vez que se mergea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants