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

[TECH] Builder pix-admin avec Embroider #6721

Merged
merged 14 commits into from
Jul 27, 2023

Conversation

nlepage
Copy link
Member

@nlepage nlepage commented Jul 26, 2023

🦄 Problème

Admin est la dernière application qui n'est pas sur embroider.

Avec Brocolli :
Le build échoue sur les erreurs de syntaxe, mais ne le signale pas.
En conséquence, le développeur doit au bout d'un moment relancer le build.

🤖 Proposition

Migrer Admin sur Embroider.

🌈 Remarques

Une 1ère tentative de migration a déjà été effectuée là : #6017

La migration introduit un problème lorsqu'on exécute les tests en mode UI (sur localhost:4202/tests), les expects sur les notifications ne échouent pas lorsque celles-ci sont en auto clear.
En attendant un possible merge de mansona/ember-cli-notifications#364, on pointe sur https://github.com/1024pix/ember-cli-notifications dans lequel l'utilisation de ember-get-config a été supprimée.

💯 Pour tester

Faire de la non régression dans toute l'appli.

Pour les erreurs de build :

  • Lancer le build.
  • Introduire une erreur de syntaxe.
  • Vérifier que le build mentionne cette erreur.
  • La corriger.
  • Vérifier que le build reprend.

@nlepage nlepage added 👀 Tech Review Needed cross-team Toutes les équipes de dev labels Jul 26, 2023
@nlepage nlepage self-assigned this Jul 26, 2023
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@nlepage nlepage force-pushed the tech-admin-embroider branch 2 times, most recently from 98c2373 to f9b164a Compare July 26, 2023 09:29
min: 1,
max: 255,
message: 'La longueur du nom ne doit pas excéder 255 caractères.',
disabled: none('model.username'),
Copy link
Contributor

Choose a reason for hiding this comment

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

On avait ici supprimer ces conditions disabled parce qu'on n'arrivait pas a les faire passer, mais il faudrait les remettre.

Copy link
Member Author

Choose a reason for hiding this comment

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

hum ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Voir 45984faaab9149780e95df298a218c3a94b03a3f pour le fix

@francois2metz
Copy link
Contributor

Pour les notifications, il y a du code spécifique pour l'env de test:

ENV['ember-cli-notifications'] = {
autoClear: null,
clearDuration: null,

@francois2metz
Copy link
Contributor

Tant qu'a faire aussi, je pense qu'il faudrait mettre à jour embroider en v3.

@nlepage
Copy link
Member Author

nlepage commented Jul 26, 2023

Pour les notifications, il y a du code spécifique pour l'env de test

Une explication et un fix là : mansona/ember-cli-notifications#364

@nlepage
Copy link
Member Author

nlepage commented Jul 26, 2023

Tant qu'a faire aussi, je pense qu'il faudrait mettre à jour embroider en v3.

wokay

@nlepage nlepage force-pushed the tech-admin-embroider branch 2 times, most recently from 5e30a15 to 8f37060 Compare July 26, 2023 13:46
@yannbertrand
Copy link
Member

OK pour le build 👍

Copy link
Contributor

@francois2metz francois2metz left a comment

Choose a reason for hiding this comment

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

🌹 🌺 💮 🥀 🌷 🌻 🌸 LGTM 🌸 🌻 🌷 🥀 💮 🌺 🌹

@pix-service-auto-merge pix-service-auto-merge merged commit 43ffd8f into dev Jul 27, 2023
5 of 6 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the tech-admin-embroider branch July 27, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants