Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Passar testos privats al repositori public #168

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

Conversation

jaumemoral
Copy link
Contributor

Despres d'una formació interna sobre testing he pensat que hauriem d'anar passant la majoria dels testos privats cap al repositori public, ja que els privats no s'estan passant automàticament i podem introduir algun bug sense voler.

He començat pels mes simples. Per ara aixo es un work in progress, aniré pujant a la branca els testos que vagi incorporant i que funcionin, de forma que si en algun moment ho voleu incorporar a master es pugui fer. Jo per ara encara no ho faria.

@jaumemoral jaumemoral force-pushed the feature_incloure_tests_privats branch from 86f1d38 to 294262f Compare October 27, 2016 09:04
@jaumemoral
Copy link
Contributor Author

Sembla que el travis es mes estricte i que testos que passen en local no passen al Travis. Queda pendent de mirar que pasa

@aaguilera
Copy link
Member

@jaumemoral entenc que en tot cas es tracta de tests que estaven al repo privat pel tema de la protecció de dades personals, però que es poden transformar en tests impersonals que reprodueixen el mateix problema, i per tant podrien ser tests públics, oi?

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

En general em sembla correcte, però jo canviaria totes les referències a @upc.edu o altres dominis existents per @example.com per fer-ho genèric de debò.

@jaumemoral
Copy link
Contributor Author

Exactament, aquesta es la idea: anonimitzar/generalitzar testos que fins ara teniem com a privats perque aixi puguin passar pel travis en cada build.

@alexm tens tota la raó

@jaumemoral
Copy link
Contributor Author

Ups, he de configurar les restriccions del PEP8 en local o després em peten els builds! :)

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

A diferència de example.com, diria que ejemplo.es no és un domini reservat per a codi d'exemple. Jo no me la jugaria posant dominis inventats que eventualment poden ser reals.

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

Novament es barregen canvis estètics i funcionals en aquest fitxer, que caldria separar en commits diferents.

@jaumemoral
Copy link
Contributor Author

example.com es un domini reservat? Igual que els telefons 555 de les pelis? Bueno, lo de ejemplo era per posar un domini diferent... Ho veus tan greu? Sobre la resta, miro que ha passat i refaig, pero primer si em podeu acceptar l'altra pull request... ho afegiré a aquesta abans de tenir conflictes

@alexm
Copy link
Member

alexm commented Nov 10, 2016

Si vas a www.example.com et mostra https://www.iana.org/domains/reserved. És a dir, sí és un nom reservat per posar en els exemples. Si vols provar-ne un de diferent, utilitza example.org, que també està reservat.

Més enllà d'això, utilitzar qualsevol altre domini no reservat amaga la intenció real de l'exemple i pot acabar donant problemes a algú, segons el tipus de test que es faci. En aquests tests és inofensiu però és una bona pràctica utilitzar els dominis oficials d'exemple enlloc d'inventar-ne de nous.

A més a més, els dominis reservats estan configurats per atendre a diferents serveis, com ara el port 80, 25, etc. Això els fa encara més interessants i reforça usar-los com a bona pràctica.

@jaumemoral
Copy link
Contributor Author

Ho sento, he d'obrir que al barrejar les branques m'he carregat algo...

@jaumemoral jaumemoral reopened this Nov 10, 2016
@jaumemoral
Copy link
Contributor Author

mmmm, crec que ja puc dir oficialment que no se que cony he fet i m'ha desaparescut tots els mails que tenia ja preparats. Seguirem informant...

@jaumemoral
Copy link
Contributor Author

Definitivament, no se com recuperar els fitxers que m'he carregat. En algun lloc del git deuen estar (suposo) pero no tinc ni idea de com tornar-hi a arribar :(

@jaumemoral
Copy link
Contributor Author

He aconseguit salvar els mobles gràcies al git reflog, recuperar els fitxers i que passi els testos

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

El test_neteja.py segueix tenint canvis d'espais barrejats amb canvis nous de funcionalitat.

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

En alguns dels missatges apareix informació de les persones (noms i cognmos, telèfon, etc.), les unitats a les que pertanyen (logos d'imatges al peu), o enllaços als tiquets de GN6.

Les dades dels tests haurien de ser totalment anònimes.

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

Aquest commit amb el canvi de nom d'un test segurament s'hauria d'incorporar via squash al primer commit del PR.

@alexm
Copy link
Member

alexm commented Nov 12, 2016

Crec que hauria de ser possible canviar el PR posant com a origen una branca del propi repo de mailtoticket, així seria més fàcil que, a banda de fer comentaris, altres persones puguessin fer canvis directament al PR.

@jaumemoral
Copy link
Contributor Author

Falta recuperar els testos associats als fitxers

attachment_signatura.txt
mail_enviat_usuari_concret.txt
mail_redirigit_usuari_concret.txt
reply-attachment.txt
urgent.txt

Estaven fents, pero es van perdre en algun moment... espero poder tirar de reflog

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

El canvis per anonimitzar s'han de fusionar (i.e. squash) amb el canvi on es van importar els missatges perquè no quedi cap rastre de les dades originals, altrament no serveix de res anonimitzar-les a posteriori.

D'altra banda, incloure canvis en el codi de test/test_neteja.py que no tenen res a veure amb l'anonimització fan que sigui més difícil de fusionar-los i queden amagats darrera del missatge del canvi.

@alexm
Copy link
Member

alexm commented Nov 13, 2016

Em sap greu posar tantes pegues a aquest PR però crec que és preferible adreçar correctament els problemes d'anonimització abans que acceptem els canvis. En aquest moment de la història del projecte, no seria una bona idea haver de rescriure-la per eliminar canvis amb dades que no haurien d'aparèixer.

Anonimitzar dades és un problema molt difícil i per aquest motiu vam crear un repositori privat en el seu moment.

@jaumemoral
Copy link
Contributor Author

He refet totalment els commits explicant la historia real dels canvis (i no dels errors de GIT) i crec que he fet totes les anonimitzacions possibles. L'unic es que he incorporat una altra vegada els canvis de salt de linia en alguns fitxers.

@alexm , si faig un rebase dels commits de la branca configurant el git correctament perque posi be els salts de linia m'ho farà bé? O hauré de fer en interactiu? I en tot cas, com faig un rebase de la meva propia branca?

Quedem-nos amb la part positiva en tot cas: passa els testos, no té conflictes a la branca i github encara no ha donat l'excepció de "Too many comments in the same pull request. Get a beer and speak face to face" :)

@jaumemoral
Copy link
Contributor Author

El rebase no ha servit, pero aixo si

git filter-branch --tree-filter 'grep -IUrl "^M" | xargs -I {} dos2unix "{}"' -f f422ce5..HEAD

@aaguilera
Copy link
Member

OMFG! @jaumemoral ets oficialment el meu ídol de Git + Grep + whatever.

@alexm
Copy link
Member

alexm commented Nov 17, 2016

Molt bona feina, @jaumemoral. Ara els canvis tenen molt millor pinta 😄

Només em queda un dubte: els blocs amb la clau PGP pública també identifiquen les persones d'aquells correus i crec que per passar el test es podrien retallar considerablement (o eliminar tot el contingut entre els marcadors fins i tot). Què en penses?

@jaumemoral
Copy link
Contributor Author

@alexm tens raó. Pensava que aquest bloc era un hash, per avui parlant m'ha comentat que es un certificat, aixi que l'he reduit a un parell de linies per no tenir informació privada

@jaumemoral
Copy link
Contributor Author

Com ho veus @alexm , la podriem acceptar?

@alexm
Copy link
Member

alexm commented Nov 22, 2016

@jaumemoral segueix havent-hi informació del propietari de la clau en la versió de text del correu. Fent gpg test/mails/pgp.txt es pot veure clarament.

@alexm
Copy link
Member

alexm commented Nov 22, 2016

Segueixo pensant que intentar anonimitzar els correus és molt més difícil que aïllar el problema concret que ha de comprovar el test i deixar només això. Entenc la necessitat de no tenir tests privats que no serveixen perquè no es comproven tant sovint, però l'esforç d'anonimitzar correus reals és massa gran, com està demostrant aquest PR.

Copy link
Member

@alexm alexm left a comment

Choose a reason for hiding this comment

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

Cal eliminar la informació de la clau PGP que conté detalls del seu propietari.

@jaumemoral
Copy link
Contributor Author

@alexm tens raó, no s'ha pujat el canvi i segueix la clau sencera. Segurament vaig fer un reset i un altre push sense fer commit dels canvis. Dijous quan estigui a la feina ho refaig

@jaumemoral
Copy link
Contributor Author

@alexm, si no m'he colat, ja estaria. Ara he vist que la clau estava 2 vegades, una en HTML i una en txt i suposo que el problema venia d'aqui

…e_reply_to

Si hi ha "reply-to" el mira abans del "from" per trobar el solicitant
@alexm
Copy link
Member

alexm commented Nov 24, 2016

@jaumemoral segueixo veient canvis de salts de línia als commits d8e1ac4 i 7ac0c40.

@alexm
Copy link
Member

alexm commented Nov 24, 2016

Tornant-ho a mirar veig que el commit fe0573f també té molts canvis de salts de línia.

@alexm
Copy link
Member

alexm commented Nov 24, 2016

Seria molt interessant trobar alguna forma de caçar aquests canvis, per posar-los al hook de pre-commit i al Travis. Així ens estalviaríem molta feina a les revisions.

@jaumemoral jaumemoral force-pushed the feature_incloure_tests_privats branch from 781b5c0 to b27d00f Compare December 4, 2016 18:48
@jaumemoral
Copy link
Contributor Author

jaumemoral commented Dec 4, 2016

He fet aixo per veure si eliminava els caracters i he posat el core.autocrlf=input

git reset --hard origin/feature_incloure_tests_privats 
git filter-branch -f --tree-filter 'git ls-files -z | xargs -0 dos2unix' 9c3473c..HEAD
git push --force

Amb aixo ho fa correcte... pero he vist que al primer commit (caa73a9) m'ha afegit un fitxer que tenia l'encoding incorrecte al GIT. Hauria de fer un split d'aquest commit, pero no me'n surto. He provat a fer un git rebase -i, marcar el commit com a edit, fer un commit ammend posant només un fitxer, fer-ne un altre amb l'altre fitxer i fer rebase --continue pero em diu que no pot aplicar el següent commit, sense més pistes :(

@alexm , alguna idea del que hauria de fer?

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

Successfully merging this pull request may close these issues.

3 participants