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.

Sign up for GitHub

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

Jump to bottom

fix(api,dashboard): déploiement du dashboard avant l'api pour éviter que la bannière 'Rafraichissez svp' ne soit inutile #1761

Merged
merged 4 commits into from
Nov 13, 2023
Merged

fix(api,dashboard): déploiement du dashboard avant l'api pour éviter que la bannière 'Rafraichissez svp' ne soit inutile #1761

merged 4 commits into from
Nov 13, 2023

Conversation

arnaudambro
Copy link
Contributor

@arnaudambro arnaudambro commented Nov 6, 2023 •
edited
Loading

je vois comme souci pour déployer le dashboard avant

  • la migration pas prête côté backend

tu vois autre chose ?

@arnaudambro
Copy link
Contributor Author

Vu que quand on clique dessus, il se passe rien, plus personne ne clique dessus, donc ça fout la merde

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 6, 2023

🎉 Deployment for commit 2a5a471 :

Ingresses
Docker images
  • 📦 docker pull harbor.fabrique.social.gouv.fr/mano/mano/api:sha-2a5a471b422df82379dfb9d31de01f17a7f6612f
  • 📦 docker pull harbor.fabrique.social.gouv.fr/mano/mano/dashboard:sha-2a5a471b422df82379dfb9d31de01f17a7f6612f
  • 📦 docker pull harbor.fabrique.social.gouv.fr/mano/mano/www:sha-2a5a471b422df82379dfb9d31de01f17a7f6612f
Debug

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

rap2hpoutre
rap2hpoutre approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@rap2hpoutre rap2hpoutre left a comment

Choose a reason for hiding this comment

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

Ok !

Mais :

  • est-ce qu'il n'y a pas d'autres fois où ça nous arrange que l'API soit déployée avant le dashboard.

Et surtout : est-ce qu'on ne devrait pas tout simplement faire deux PR quand on a un truc qui dépend de l'autre ? Moi c'est mon approche préférée.

Dans le cas où le pb est une migration : on livre la route de migration côté backend, puis le front.

Ça fait du code en moins (car finalement à chaque fois qu'on ajoute du code, on diminue la stabilité de notre application et on augmente la complexité ; là on aura maintenant des nouveaux bugs complexe à comprendre à cause de migrations available qu'on aura oublié, raté, mal compris, etc.)

api/src/controllers/migration.js Outdated Show resolved Hide resolved
@arnaudambro
Copy link
Contributor Author

Et surtout : est-ce qu'on ne devrait pas tout simplement faire deux PR quand on a un truc qui dépend de l'autre ? Moi c'est mon approche préférée.

on peut, pas de problème, mais ce n'est pas là le souci

le souci c'est:

  1. l'api déploie - sans aucun changement - mais elle s'attend à un dashboard vN+1 - le dashboard est vN - une bannière apparait
  2. l'utilisateur clique sur la bannière, et elle reste là
  3. le dashboard vN+1 est déployé - tout va bien

donc ce n'est pas lié à une stratégie d'écriture de code, mais vraiment un truc d'ops et de machine etc.

Arnaud AMBROSELLI and others added 3 commits November 13, 2023 15:30
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@arnaudambro
Copy link
Contributor Author

ahhhh
mais j'avais pas vu mon code bon dieu
ok tu as raison
quand on fait une migration, on fait d'iun côté le back, de l'autre le front
top

@arnaudambro arnaudambro merged commit 0da83b4 into main Nov 13, 2023
5 of 7 checks passed
@arnaudambro arnaudambro deleted the fix/change-deployment-order branch November 13, 2023 14:33
SocialGroovyBot added a commit that referenced this pull request Nov 13, 2023
@SocialGroovyBot
chore(release): version 1.293.4
cc93fd5 
## [1.293.4](v1.293.3...v1.293.4) (2023-11-13)

### Bug Fixes

* **api,dashboard:** déploiement du dashboard avant l'api pour éviter que la bannière 'Rafraichissez svp' ne soit inutile ([#1761](#1761)) ([0da83b4](0da83b4))
* **dashboard:** rajout du bouton Rafraichir ([#1760](#1760)) ([07eeacb](07eeacb))
@SocialGroovyBot
Copy link
Member

🎉 This PR is included in version 1.293.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rap2hpoutre rap2hpoutre rap2hpoutre approved these changes

Assignees

@rap2hpoutre rap2hpoutre

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arnaudambro @SocialGroovyBot @rap2hpoutre