[TECH] Utiliser la version de release qui vient d'être crée plutôt que d'appeler l'API Github #236

mickaelalibert · 2023-03-20T16:05:27Z

🦄 Problème

Lorsque nous créons une nouvelle release, la version à déployer est récupérée depuis l'API de GitHub plutôt que d'utiliser la version tout juste créée, ce qui peut provoquer des incohérences (release créée mais autre version déployée)

🤖 Proposition

Utiliser la version qui vient d'être release

💯 Pour tester

Vérifier la CI

pix-bot-github · 2023-03-20T16:05:42Z

Une fois l'application déployée, elle sera accessible à cette adresse https://bot-pr236.review.pix.fr
Les variables d'environnement seront accessibles sur scalingo https://dashboard.scalingo.com/apps/osc-fr1/pix-bot-review-pr236/environment

yoandl

LGTM

test/unit/run/services/slack/commands_test.js

octo-topi · 2023-03-23T16:25:23Z

common/services/releases.js

@@ -54,7 +54,8 @@ module.exports = {
      const branchName = await github.getDefaultBranch(config.github.owner, sanitizedRepoName);
      const repositoryURL = `https://${config.github.token}@github.com/${config.github.owner}/${sanitizedRepoName}.git`;
      const args = [config.github.owner, sanitizedRepoName, sanitizedReleaseType, branchName, repositoryURL];
-      await _runScriptWithArgument(RELEASE_PIX_SCRIPT, ...args);
+      const newPackageVersion = await _runScriptWithArgument(RELEASE_PIX_SCRIPT, ...args);
+      return newPackageVersion;


Retourner newPackageVersion en dehors du try/catch.
Mieux: supprimer le try/catch et laisser HapiJs gérer l'erreur non catchée, ou poster un message dans Slack pour dire que la release a fail

Il manque un test sur la valeur retournée dans test/unit/common/services/releases_test.js - "#publishPixRepo"

Retourner newPackageVersion en dehors du try/catch.

Je ne vois pas ce que cela changerais. Peut tu expliciter ? (même après merge)

Mieux: supprimer le try/catch et laisser HapiJs gérer l'erreur non catchée, ou poster un message dans Slack pour dire que la release a fail

Il s'agit d'un tout autre problème a traiter hélas, hors du scope de cette PR.

octo-topi · 2023-03-23T16:32:03Z

test/unit/run/services/slack/commands_test.js

@@ -19,12 +19,12 @@ const releasesServices = require('../../../../../common/services/releases');
 const githubServices = require('../../../../../common/services/github');
 const ScalingoClient = require('../../../../../common/services/scalingo-client');

-describe('Services | Slack | Commands', () => {
+describe('Unit | Run | Services | Slack | Commands', () => {


octo-topi · 2023-03-23T16:45:57Z

run/services/slack/commands.js

@@ -64,13 +64,13 @@ function getErrorAppMessage(appName) {
 function _isReleaseTypeInvalid(releaseType) {
  return !['major', 'minor', 'patch'].includes(releaseType);
 }
-async function publishAndDeployPixUI(repoName, releaseType, responseUrl) {
+
+async function publishPixUI(repoName, releaseType, responseUrl) {


octo-topi · 2023-03-23T17:01:27Z

test/unit/run/services/slack/commands_test.js

@@ -106,7 +101,7 @@ describe('Services | Slack | Commands', () => {
      await createAndDeployPixUI(payload);

      // then
-      sinon.assert.calledWith(githubServices.getLatestReleaseTag, 'pix-ui');
+      sinon.assert.calledOnceWithExactly(githubServices.getLatestReleaseTag, 'pix-ui');


Il y a un problème de fond ici dans ce que teste ce fichier nommé "test unitaire"

Pour moi, ce test unitaire doit vérifier que createAndDeployPixUI a appelé
publishPixUI. Ici, il vérifie que publishPixUI a appelé githubServices.getLatestReleaseTag, ce qui devrait être fait uniquement dans le TU de publishPixUI (sauf qu'il n'y a pas de TU dessus). Le code est couvert par les tests, mais par une cascade d'appels, ce qui n'est pas top..

Après investigation, publishPixUI est une fonction interne au fichier. Du coup cela ne me choque pas. Nous avons préfixer ces fonctions par un underscore pour clarifier cela.

bin/www

octo-topi · 2023-03-23T17:16:41Z

Comme tu as demandé un review approfondie, j'inclus le découpage en commit.

Voilà les commits

 :fire: Remove unnecessary tests
 :sparkles: Migrating last release usecase
 :truck: Rename Pix Ui publish function as it is not deploying anything
 :sparkles: Update test following rework
 :bug: Fix incorrect test before patch
:sparkles: Use last version returned from release
 :sparkles: release pix repo now outputs released version
 :sparkles: use generated released tag instead of fetching Github API

J'observe plusieurs choses:

il y a des commits sans tests: quand j'ajoute une fonctionnalité, je groupe dans un même commit tests et implém
il y a des modifications non compatibles dans le temps: l'appel de publish dans view-submission est récupéré dans le premier commit, alors que publish ne renvoie encore rien - j'aurais modifié dans l'ordre
- release-pix-repo.sh
- releases.js / publish
- view-submission.js / submitReleasePublicationConfirmation
il y a 2 points d'entrée fonctionnel ( dans Slack) différents : publishPixRepo (?) et pix-ui (publishAndDeployPixUI) modifiés dans un commit : j'aurai fait deux commits qui mentionnent le point d'entrée, ex: "fix potentially invalid release in pix-ui deployment"
dans le même objectif, le commit ":sparkles: Migrating last release usecase" pourrait être renommé pour expliciter le point d'entrée fonctionnel "PixSite" et être splitté en deux, la version Slack sur demande et la version Job planifiée
le commit "Use last version returned from release" ajoute le test "should not retrieve the last release tag from GitHub" , et le commit "Remove unnecessary tests" le supprime. Je devine que tu veux ajouter un test au comportement problématique avant de le modifier. Soit. Je pense que c'est inutile. Mais si tu veux le faire, alors fais le isolément dans un commit (pas avec la correction) au tout début de la branche, ex : "Add missing test xxxxxx "
les commits suivants modifient la même ligne du même fichier de test et c'est tout: je fusionnerai les deux commits (et j'incluerai l'implémentation)
" ✨ Update test following rework" + " 🐛 Fix incorrect test before patch"

octo-topi

Le serveur démarre mais pas le job de deploy pix site, voir https://github.com/1024pix/pix-bot/pull/236/files#r1146522740

francois2metz

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

old

mickaelalibert added team-captains This is your captain speaking Development in progress labels Mar 20, 2023

mickaelalibert self-assigned this Mar 20, 2023

mickaelalibert force-pushed the fix-slack-deploy-version branch 2 times, most recently from 3c27130 to 8d450bf Compare March 21, 2023 13:33

mickaelalibert changed the title ~~✨ use generated released tag instead of fetching Github API~~ [TECH] Utiliser la version de release qui vient d'être créer plutôt que d'appeler l'API Github Mar 22, 2023

mickaelalibert added 👀 Tech Review Needed and removed Development in progress labels Mar 22, 2023

yoandl approved these changes Mar 22, 2023

View reviewed changes

test/unit/run/services/slack/commands_test.js Outdated Show resolved Hide resolved

octo-topi changed the title ~~[TECH] Utiliser la version de release qui vient d'être créer plutôt que d'appeler l'API Github~~ [TECH] Utiliser la version de release qui vient d'être crée plutôt que d'appeler l'API Github Mar 23, 2023

octo-topi reviewed Mar 23, 2023

View reviewed changes

bin/www Outdated Show resolved Hide resolved

octo-topi previously requested changes Mar 23, 2023

View reviewed changes

mickaelalibert added 3 commits March 27, 2023 10:56

✨ use generated released tag instead of fetching Github API

1f11c41

✨ release pix repo now outputs released version

afe554c

✨ Use last version returned from release

935497c

francois2metz force-pushed the fix-slack-deploy-version branch 2 times, most recently from 7bfcc35 to 0ea7ca1 Compare March 27, 2023 09:26

mickaelalibert and others added 7 commits March 27, 2023 11:30

🐛 Fix incorrect test before patch

ec08456

🚚 Rename Pix Ui publish function as it is not deploying anything

56dc78e

✨ Migrating last release usecase

0357d2c

🔥 Remove unnecessary tests

8f76211

Add test on publishPixRepo

da526c4

Extract variable repoName

cdbc3be

Prefix all internal functions with an underscore

25e96d4

francois2metz force-pushed the fix-slack-deploy-version branch from 0ea7ca1 to 25e96d4 Compare March 27, 2023 09:31

francois2metz approved these changes Mar 27, 2023

View reviewed changes

francois2metz requested a review from octo-topi March 27, 2023 09:32

francois2metz added 🚀 Ready to Merge Tech Review OK and removed 👀 Tech Review Needed labels Mar 27, 2023

francois2metz added 🚀 Ready to Merge and removed 🚀 Ready to Merge labels Mar 27, 2023

github-actions bot merged commit 1809e19 into main Mar 27, 2023

github-actions bot deleted the fix-slack-deploy-version branch March 27, 2023 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TECH] Utiliser la version de release qui vient d'être crée plutôt que d'appeler l'API Github #236

[TECH] Utiliser la version de release qui vient d'être crée plutôt que d'appeler l'API Github #236

mickaelalibert commented Mar 20, 2023 •

edited by octo-topi

Loading

pix-bot-github commented Mar 20, 2023

yoandl left a comment

octo-topi Mar 23, 2023

octo-topi Mar 23, 2023

francois2metz Mar 27, 2023 •

edited

Loading

octo-topi Mar 23, 2023

octo-topi Mar 23, 2023

octo-topi Mar 23, 2023 •

edited

Loading

francois2metz Mar 27, 2023

octo-topi commented Mar 23, 2023

octo-topi left a comment

francois2metz left a comment

[TECH] Utiliser la version de release qui vient d'être crée plutôt que d'appeler l'API Github #236

[TECH] Utiliser la version de release qui vient d'être crée plutôt que d'appeler l'API Github #236

Conversation

mickaelalibert commented Mar 20, 2023 • edited by octo-topi Loading

🦄 Problème

🤖 Proposition

💯 Pour tester

pix-bot-github commented Mar 20, 2023

yoandl left a comment

Choose a reason for hiding this comment

octo-topi Mar 23, 2023

Choose a reason for hiding this comment

octo-topi Mar 23, 2023

Choose a reason for hiding this comment

francois2metz Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

octo-topi Mar 23, 2023

Choose a reason for hiding this comment

octo-topi Mar 23, 2023

Choose a reason for hiding this comment

octo-topi Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

francois2metz Mar 27, 2023

Choose a reason for hiding this comment

octo-topi commented Mar 23, 2023

octo-topi left a comment

Choose a reason for hiding this comment

francois2metz left a comment

Choose a reason for hiding this comment

mickaelalibert commented Mar 20, 2023 •

edited by octo-topi

Loading

francois2metz Mar 27, 2023 •

edited

Loading

octo-topi Mar 23, 2023 •

edited

Loading