entrouvert
/
authentic
16
0
Fork 0
Code Issues Pull Requests 32 Releases Activity

nettoyage des applications obsolètes/squash des migrations (#40685) (série partielle) #262

Open
bdauvergne wants to merge 9 commits from wip/40685-squash-des-migrations-partial into main
pull from: wip/40685-squash-des-migrations-partial
merge into: entrouvert:main
Conversation 4 Commits 9 Files Changed 106 +190 -3119
bdauvergne commented 2024-02-27 18:26:02 +01:00
Owner
Copy Link
No description provided.
bdauvergne added 9 commits 2024-02-27 18:26:02 +01:00
b080c03f07 misc: force check-migrations to use en language (#40685) 
It prevents having translated messages in migrations.
3f9829d261 misc: unplug auth_migrations_18 (#40685) 
Native django.contrib.auth migrations can be used now, the only glitch
are permissions on the proxy model LDAPUser:

    A problem arose migrating proxy model permissions for custom_user_user to authentic2_ldapuser.

      Permission(s) for authentic2_ldapuser already existed.
      Codenames Q: (AND: ('codename__in', ['add_ldapuser', 'change_ldapuser', 'delete_ldapuser', 'view_ldapuser']))

    Ensure to audit ALL permissions for custom_user_user and authentic2_ldapuser.

It can be fixed with the following SQL command:

   DELETE FROM auth_permission WHERE content_type_id = (SELECT id FROM django_content_type WHERE model = 'ldapuser' AND app_label = 'authentic2');
785e28d93a misc: unplug attribute_aggregator (#40685) 
All models were removed before.
0a81164beb misc: unplug authentic2.idp (#40685) 
All models were removed before.
110d5073b9 misc: remove all use of a get_user_model (#40685) 
It should only be used in reusable library.
9fba0d757f misc: remove use of settings.AUTH_USER_MODEL (#40685) 
To rewrite authentic migrations we need to cut ties with
django.contrib.auth, it's simpler if AUTH_USER_MODEL is not used inside
authentic by only by django.contrib.auth code dependant upon the
effective User model. Authentic's code should directly reference
custom_user.User.
gitea/authentic/pipeline/head There was a failure building this commit Details
9b73eb7066 tests: remove migration tests (#40685) 
Test on old migrations are useless and they will break after squashing
all migrations.
bdauvergne force-pushed wip/40685-squash-des-migrations-partial from 9b73eb7066 to 166823087a 2024-02-27 18:32:10 +01:00 Compare
bdauvergne changed title from WIP: nettoyage des applications obsolètes/squash des migrations (#40685) (série partielle) to nettoyage des applications obsolètes/squash des migrations (#40685) (série partielle) 2024-02-27 18:39:15 +01:00
bdauvergne requested review from pmarillonnet 2024-02-27 18:39:22 +01:00
pmarillonnet commented 2024-03-14 15:48:06 +01:00
Owner
Copy Link

(Je commence à relire.)

(Je commence à relire.)
pmarillonnet reviewed 2024-03-21 16:13:52 +01:00
pmarillonnet left a comment
Owner
Copy Link

Une première relecture, quelques trucs vu au passage.

Aussi je n’ai compris ce que le retrait des tests de migration venait faire ici (tout le dernier commit 166823087a). Je pense qu’il vaudrait mieux reporter ce retrait à la PR où ces migrations seraient effectivement squashées ; à l’inverse je ne suis pas trop fan de supprimer ça en prévention dans cette PR alors que les migrations concernées, elles, sont toujours là.

De mon côté je vais taper un branche wip d’hobo dont la CI ciblerait cette branche authentic pour m’assurer que ça tourne. Je vais faire un tour des plugins a2 aussi pour être sûr qu’on y utilise pas encore une des applis supprimées ici.

Une première relecture, quelques trucs vu au passage. Aussi je n’ai compris ce que le retrait des tests de migration venait faire ici (tout le dernier commit 166823087a5). Je pense qu’il vaudrait mieux reporter ce retrait à la PR où ces migrations seraient effectivement squashées ; à l’inverse je ne suis pas trop fan de supprimer ça en prévention dans cette PR alors que les migrations concernées, elles, sont toujours là. De mon côté je vais taper un branche wip d’hobo dont la CI ciblerait cette branche authentic pour m’assurer que ça tourne. Je vais faire un tour des plugins a2 aussi pour être sûr qu’on y utilise pas encore une des applis supprimées ici.
check-migrations.sh Outdated
@ -6,6 +6,7 @@ set -e
CHECK_MIGRATIONS_SETTINGS=`mktemp`
trap "rm -f ${CHECK_MIGRATIONS_SETTINGS}" EXIT
cat <<EOF >${CHECK_MIGRATIONS_SETTINGS}
LANGUAGE_CODE = 'en'
pmarillonnet commented 2024-03-21 14:42:02 +01:00
Owner
Copy Link

Par curiosité, qu’est-ce ce qui se passe mal dans check-migrations si on ne pose pas ça ?

Par curiosité, qu’est-ce ce qui se passe mal dans `check-migrations` si on ne pose pas ça ?
bdauvergne commented 2024-03-21 16:15:43 +01:00
Author
Owner
Copy Link

Si les locales sont compilées, on se retrouve avec erreurs de migrations manquante parce que verbose_name/help_text ne correspondent plus.

Si les locales sont compilées, on se retrouve avec erreurs de migrations manquante parce que verbose_name/help_text ne correspondent plus.
pmarillonnet commented 2024-03-21 16:28:45 +01:00
Owner
Copy Link

Ok

Ok
pmarillonnet marked this conversation as resolved
src/authentic2/auth_migrations_18/0001_initial.py Outdated
@ -48,4 +0,0 @@
},
),
migrations.CreateModel(
name='User',
pmarillonnet commented 2024-03-21 15:03:09 +01:00
Owner
Copy Link

Ok, j’ignorais que auth_migrations_18 avait défini autant de modèles à un moment. Dégageons tout cela oui.

Ok, j’ignorais que `auth_migrations_18` avait défini autant de modèles à un moment. Dégageons tout cela oui.
pmarillonnet marked this conversation as resolved
src/authentic2/idp/interactions.py Outdated
@ -26,4 +0,0 @@
if request.method == 'GET':
return render(
request,
'interaction/consent_federation.html',
pmarillonnet commented 2024-03-21 15:39:50 +01:00
Owner
Copy Link

On pourrait en profiter pour dégager ce gabarit aussi.

On pourrait en profiter pour dégager ce gabarit aussi.
bdauvergne commented 2024-03-21 16:17:51 +01:00
Author
Owner
Copy Link

Bonne idée.

Bonne idée.
pmarillonnet marked this conversation as resolved
src/authentic2/settings.py
@ -153,9 +153,7 @@ INSTALLED_APPS = (
'authentic2_idp_oidc',
'authentic2.nonce',
pmarillonnet commented 2024-03-21 16:01:57 +01:00
Owner
Copy Link

Est-ce qu’on pourrait pas aussi dégager cette appli de la déclaration des applis installées ?

Est-ce qu’on pourrait pas aussi dégager cette appli de la déclaration des applis installées ?
bdauvergne commented 2024-03-21 16:18:15 +01:00
Author
Owner
Copy Link

Pas encore sinon la migration delete_nonce ne s'appliquera pas et on aura une table qui traîne.

Pas encore sinon la migration delete_nonce ne s'appliquera pas et on aura une table qui traîne.
pmarillonnet commented 2024-03-21 16:29:13 +01:00
Owner
Copy Link

Ah oui ok, bien vu.

Ah oui ok, bien vu.
pmarillonnet marked this conversation as resolved
src/authentic2/validators.py Outdated
@ -30,3 +30,3 @@
# copied from http://www.djangotips.com/real-email-validation
@deconstructible
pmarillonnet commented 2024-03-21 16:02:49 +01:00
Owner
Copy Link

J’ai pas capté ce qui justifiait d’ajouter ce décorateur là maintenant, dans cette PR :/

J’ai pas capté ce qui justifiait d’ajouter ce décorateur là maintenant, dans cette PR :/
bdauvergne commented 2024-03-21 16:21:11 +01:00
Author
Owner
Copy Link

C'est la recréation de la migration pour le modèle User qui utiliser EmailValidator et sans le décorateur deconstructible make_migration n'accepter pas de la refaire, problème invisible avant (et je suppose que l'introduction de EmailValidator s'est faite en monkeypatchant les migrations existantes donc on l'a pas vu non plus au passage).

Donc c'est utile, mais plus loin dans l'autre série de commits.

C'est la recréation de la migration pour le modèle User qui utiliser EmailValidator et sans le décorateur deconstructible make_migration n'accepter pas de la refaire, problème invisible avant (et je suppose que l'introduction de EmailValidator s'est faite en monkeypatchant les migrations existantes donc on l'a pas vu non plus au passage). Donc c'est utile, mais plus loin dans l'autre série de commits.
pmarillonnet commented 2024-03-21 16:32:07 +01:00
Owner
Copy Link

Ok.

Ok.
pmarillonnet marked this conversation as resolved
bdauvergne force-pushed wip/40685-squash-des-migrations-partial from 166823087a to 03a79037f5 2024-03-21 16:25:18 +01:00 Compare
bdauvergne commented 2024-03-21 16:26:43 +01:00
Author
Owner
Copy Link

J'ai retiré deux templates inutilisés src/authentic2/templates/interaction/consent_attributes.html et src/authentic2/templates/interaction/consent_federation.html dans le commit "misc: unplug authentic2.idp (#40685)".

J'ai retiré deux templates inutilisés src/authentic2/templates/interaction/consent_attributes.html et src/authentic2/templates/interaction/consent_federation.html dans le commit "misc: unplug authentic2.idp (#40685)".
pmarillonnet commented 2024-03-21 16:34:30 +01:00
Owner
Copy Link

J'ai retiré deux templates inutilisés src/authentic2/templates/interaction/consent_attributes.html et src/authentic2/templates/interaction/consent_federation.html dans le commit "misc: unplug authentic2.idp (#40685)".

Ok, top.

Et donc il reste la partie de mon commentaire principal :

Aussi je n’ai compris ce que le retrait des tests de migration venait faire ici (tout le dernier commit 166823087a). Je pense qu’il vaudrait mieux reporter ce retrait à la PR où ces migrations seraient effectivement squashées ; à l’inverse je ne suis pas trop fan de supprimer ça en prévention dans cette PR alors que les migrations concernées, elles, sont toujours là.

Est-ce qu’on pourrait déplacer ce commit dans la série de commits suivants, et passer ici les huit premiers seulement ?

> J'ai retiré deux templates inutilisés src/authentic2/templates/interaction/consent_attributes.html et src/authentic2/templates/interaction/consent_federation.html dans le commit "misc: unplug authentic2.idp (#40685)". Ok, top. Et donc il reste la partie de mon commentaire principal : > Aussi je n’ai compris ce que le retrait des tests de migration venait faire ici (tout le dernier commit 166823087a). Je pense qu’il vaudrait mieux reporter ce retrait à la PR où ces migrations seraient effectivement squashées ; à l’inverse je ne suis pas trop fan de supprimer ça en prévention dans cette PR alors que les migrations concernées, elles, sont toujours là. Est-ce qu’on pourrait déplacer ce commit dans la série de commits suivants, et passer ici les huit premiers seulement ?
pmarillonnet commented 2024-03-21 16:51:19 +01:00
Owner
Copy Link

De mon côté je vais taper un branche wip d’hobo dont la CI ciblerait cette branche authentic pour m’assurer que ça tourne. Je vais faire un tour des plugins a2 aussi pour être sûr qu’on y utilise pas encore une des applis supprimées ici.

Un truc se passe mal (https://jenkins.entrouvert.org/job/gitea/job/hobo/job/wip%252Ftmp-paul-check-ci-against-a2-migrations-squash/1/console)

django.core.exceptions.ImproperlyConfigured: Application labels aren't unique, duplicates: saml

> De mon côté je vais taper un branche wip d’hobo dont la CI ciblerait cette branche authentic pour m’assurer que ça tourne. Je vais faire un tour des plugins a2 aussi pour être sûr qu’on y utilise pas encore une des applis supprimées ici. Un truc se passe mal (https://jenkins.entrouvert.org/job/gitea/job/hobo/job/wip%252Ftmp-paul-check-ci-against-a2-migrations-squash/1/console) > django.core.exceptions.ImproperlyConfigured: Application labels aren't unique, duplicates: saml
All checks were successful
gitea/authentic/pipeline/head This commit looks good
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b wip/40685-squash-des-migrations-partial main
git pull origin wip/40685-squash-des-migrations-partial

Step 2:

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff wip/40685-squash-des-migrations-partial
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
pmarillonnet
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/authentic#262
No description provided.
Delete Branch "wip/40685-squash-des-migrations-partial"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?