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

Revert de la lib GeoIp #2603

Closed
wants to merge 3 commits into from
Closed

Conversation

firm1
Copy link
Contributor

@firm1 firm1 commented Apr 28, 2015

Q R
Correction de bugs ? oui
Nouvelle Fonctionnalité ? non
Tickets (issues) concernés #2600 , #2483

Cette PR, permet de revenir à l'état d'avant, c'est à dire utiliser pygeoip à la place de geoip (introduit récemment dans le code). Les raisons du rollback sont évoqués dans la discussion sur le forum.

La PR règle donc par conséquent les régressions introduites avec ce changement de lib.

Note pour QA :

  • Connectez vous si le site depuis une adresse IP en ipv6 et vérifiez dans votre page de profil que la localisation est bien faite. Vous devez au moins voir votre pays

PS : a squasher avant le merge, mais la QA peut être faite sans souci

@landscape-bot
Copy link

Code Health
Repository health decreased by 1% when pulling 9716272 on firm1:revert-geoip into 4fa0c03 on zestedesavoir:release-v1.8.

@SpaceFox
Copy link
Contributor

Je repose ma question ici : pourquoi 3 commits alors que normalement ça devrait être le revert d'un commit ?

@firm1
Copy link
Contributor Author

firm1 commented Apr 28, 2015

Je repose ma question ici : pourquoi 3 commits alors que normalement ça devrait être le revert d'un commit ?

parce que "github". d'où ma petite remarque à la fin de la PR "a squasher avant le merge, mais la QA peut être faite sans souci"

@SpaceFox
Copy link
Contributor

parce que "github". d'où ma petite remarque à la fin de la PR "a squasher avant le merge, mais la QA peut être faite sans souci"

Mauvaise réponse. Si on revert un commit, on devrait avoir un commit de revert propre, pas un machin fait à la main dans GitHub.

@SpaceFox
Copy link
Contributor

PS : pas de réponse convaincante = fermeture de la PR.

@firm1
Copy link
Contributor Author

firm1 commented Apr 28, 2015

Mauvaise réponse. Si on revert un commit, on devrait avoir un commit de revert propre, pas un machin fait à la main dans GitHub.

Sauf que là, c'est un revert de 2 commit. Bonne réponse ? :stress:

@GerardPaligot
Copy link
Member

J'en suis déjà à mon deuxième pop corn !

@SpaceFox
Copy link
Contributor

Sauf que là, c'est un revert de 2 commit. Bonne réponse ? :stress:

Ça expliquerait 2 commits, pas 2. Et ça n'explique pas pourquoi il manque les IDs des commits revertés.

Un peu de doc à appliquer, je suis sûr qu'il y a la fonctionnalité dans ton client Git préféré.

@SpaceFox SpaceFox closed this Apr 28, 2015
@firm1
Copy link
Contributor Author

firm1 commented Apr 28, 2015

Arf ... Mais en quoi faire un git revert permet de mieux corriger le problème ?

J'ai comme l'impression que tu n'es pas de bonne humeur aujourd'hui :)

@SpaceFox
Copy link
Contributor

  1. Être sûr que ça ne reverte que le commit problématique et tout le commit problématique
  2. Comprendre à la lecture de l'historique que ce qui est fait ici est le revert volontaire et complet d'une fonctionnalité implémentée auparavant.

PS : j'étais plutôt de bonne humeur jusqu'à ce que je voie arriver tes 2 PR.

@firm1
Copy link
Contributor Author

firm1 commented Apr 28, 2015

Oo ! Tant de mauvaise fois ...

@Eskimon
Copy link
Contributor

Eskimon commented Apr 28, 2015

En toute impartialité je me place du côté de @SpaceFox. Un revert c'est fait par un git revert <sha-du-commit> et apparaitra proprement comme Revert moncommit dans l'historique. Ne pas faire ça s'est s'exposer à des incompréhension le jour où on voudra faire un revert du revert pour remettre la feature (car oui, ça pourrait arriver)

@Eskimon
Copy link
Contributor

Eskimon commented Apr 30, 2015

J'ai refait comme @firm1 mais à coup de git-revert, je pense n'avoir rien oublié... #2609

@firm1 firm1 deleted the revert-geoip branch June 24, 2015 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants