Skip to content
Snippets Groups Projects

Json output

Open poka requested to merge json-output into master
1 unresolved thread

pour généraliser les sorties json sur toutes les commandes

Merge request reports

Pipeline #40790 passed

Pipeline passed for c7e0404b on json-output

Ready to merge by members who can write to the target branch.
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • poka requested review from @Nicolas80

    requested review from @Nicolas80

  • assigned to @pokapow

  • Author Owner

    basé sur la branche choose-ed25519

  • Really nice to support JSON output for all of those (as long as we don't put the JSON format by default :smiley:)

  • Nicolas80 approved this merge request

    approved this merge request

  • Nicolas80 mentioned in issue #47

    mentioned in issue #47

  • Nicolas80 added 12 commits

    added 12 commits

    • 8760c3bd...3738e8b7 - 2 earlier commits
    • 44073020 - * Adding `-c` crypto scheme parameter (with default ed25519) in different places
    • da7c790d - * Added some logic between arguments of `vault import`; can't provide both...
    • 8bf81cbc - * Adapted catch_known of predefined derivations to properly handle both...
    • 0da71079 - * reverted most of the commit e9cd6a99: "Add secret format in database and display"
    • bbd95707 - Added extra message when the DB parsing of DbAccountId fails, so we know for...
    • f9beaf54 - * Updated version to 0.4.1
    • 2843da7b - use json output for all usefull commands
    • c4f8f3dd - Fixes after rebase of modified upstream branch
    • f72566bd - * changes from `cargo fmt`
    • a6bc5180 - * small fix for `vault use` with Json output format.

    Compare with previous version

  • Nicolas80 added 5 commits

    added 5 commits

    • bb54cc00 - 1 commit from branch master
    • e4b8f061 - use json output for all usefull commands
    • a094482d - changes from `cargo fmt`
    • 63eed5bd - * small fix for `vault use` with Json output format.
    • 503d4caa - Adapting `vault inspect`

    Compare with previous version

  • Nicolas80 added 7 commits

    added 7 commits

    Compare with previous version

  • Nicolas80 added 1 commit

    added 1 commit

    • 7b04c93a - Added simple tests of display::compute_vault_accounts_json method

    Compare with previous version

  • poka added 4 commits

    added 4 commits

    • fce724fd - use json output for all usefull commands
    • 81226792 - changes from `cargo fmt`
    • 64e547b4 - * small fix for `vault use` with Json output format.
    • 6be7a4d9 - Adapting `vault inspect`

    Compare with previous version

  • poka added 1 commit

    added 1 commit

    • f4e7026a - Added simple tests of display::compute_vault_accounts_json method

    Compare with previous version

  • Nicolas80 added 1 commit

    added 1 commit

    • 4e7c3ed7 - Added new version 0.4.3-RC1 changelogs for Release Candidate.

    Compare with previous version

  • Nicolas80 added 1 commit

    added 1 commit

    • cdc129ad - Added new version 0.4.3-RC1 changelogs for Release Candidate.

    Compare with previous version

    • @HugoTrentesaux est-ce que tu veux faire un review de celle-ci avant merge ?

      J'ai fais une release candidate postée sur le forum et je comptais attendre quelques jours avant de merge.

    • TL;DR : je désapprouve mais ne m'oppose pas

      Je suis vraiment pas fan de la manière dont c'est implémenté. Plutôt que d'avoir un match(output_format) dans chaque command, j'aurais changé la valeur de retour pour un Result typé. Le type retourné doit implémenter les traits serialize_json (implémenté automatiquement) et serialize_human (implémenté manuellement soit avec un format!, soit avec des println!, mais proprement).

      Ça implique de faire un type pour chaque valeur de retour, mais ça simplifie pas mal le développement.

      Mais comme je n'ai pas eu le temps de faire ce retour pendant les deux mois où la MR était ouverte, je ne vais pas exprimer un avis bloquant.

    • Author Owner

      Je crois que j'avais juste repris la façon de faire déjà en place ailleurs dans le code pour la généraliser aux autres commande. Ce que tu proposes me semble être un refac global de la sortie json.

    • Oui, c'est ma faute, j'aurais pas dû merger la PoC que j'avais fait pour le support json, ça donne une mauvaise idée de comment faire :sweat_smile:

    • Pour le serialize_json dont tu parle, j'imagine qu'il faudrait également prévoir un objet spécifique (à sérialiser en json) en cas d'erreur, un peu comme pour les API Rest avec le media type "application/problem+json".

      Exemple venant d'un site fédéral belge qui donne notament des guidelines pour les API Rest:

      https://www.belgif.be/specification/rest/api-guide/#error-handling

      Bon, je ne sais pas trop si on veux prendre la même chose ici (ce n'est pas à proprement parler des API "Rest" :slight_smile:)

      Sinon, on fait quoi du coup pour cette MR; est-ce que @pokapow ou moi tentons de faire le changement ou ... ?

      Edited by Nicolas80
    • @HugoTrentesaux, @pokapow Est-ce que vous pourriez review le dernier commit 31cba144 ?

      J'ai appliqué les changements suggérés par Hugo pour la commande vault inspect.

      A voir si cela vous semble correct avant de regarder pour adapter pour les autres commandes qui supportent le OutputFormat json.

      Exemple avec ce changement quand une GcliError est retournée:

      format human (pas de changement)

      gcli -o human vault inspect -v predef-sr//Aliceaze --no-password
      Input("No account found with name:'predef-sr' and path:'//Aliceaze'")

      format json => converti en JsonOutputError et sérialisé en json

      gcli -o json vault inspect -v predef-sr//Aliceaze --no-password | jq
      {
        "error_type": "Input",
        "error": "No account found with name:'predef-sr' and path:'//Aliceaze'"
      }
    • Qu'est-ce qui t'empêche de gérer l'affichage au plus haut niveau :

      let result = match data.args.subcommand.clone() { /* ... */ };
      print_command_output(data.args.output_format, result);

      Avec le match sur le output format à l'intérieur en cas d'erreur également ?

      À part l'async, peut-être ?

      Comme ça, tout ce qu'on attend des commandes c'est de retourner quelque chose qui se sérialise. Pas trop le temps de regarder ça plus en détail, mais la MR me semble aller dans la bonne direction en terme de lisibilité et maintenabilité.

    • Please register or sign in to reply
  • Nicolas80 added 1 commit

    added 1 commit

    • 31cba144 - * Added SerializeJson & SerializeHuman traits

    Compare with previous version

  • Nicolas80 added 2 commits

    added 2 commits

    • c5354176 - * Added json serialization for `vault remove` command using the new system.
    • 55fa2204 - * Extra changes for better json output support

    Compare with previous version

  • Nicolas80 added 1 commit

    added 1 commit

    • c7e0404b - * Adapted several commands to the new structure with SerializeHuman

    Compare with previous version

Please register or sign in to reply
Loading