fix: unbreak the collection against current DVLS - #34
Conversation
…osable The playbooks read entries that had to already exist on the test server, including a hardcoded entry id, so the suite broke whenever that server drifted. Most tasks also only printed their result instead of asserting anything. Each play now creates what it reads under a name unique to the run, asserts the values it wrote, and deletes its entries afterwards.
Updating an existing secret failed with a bare 400: DVLS now requires the tags field on a PUT. Updates resend the tags the entry already has. Any login failure surfaced as "Module result deserialization failed" on ansible-core 2.19, because the finally block called logout with a token that was never assigned. This is the February support case, invisible on 2.18. DVLS error messages are reported instead of only the status code, and the entry payload matches the public API (subType, non null description).
There was a problem hiding this comment.
🟡 Changes recommended
Tests retain external-state dependencies, expose cleanup tokens, and do not cover key regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves DVLS integration-test reliability while fixing secret updates and login-error handling.
Changes:
- Creates run-specific test secrets with assertions and cleanup.
- Preserves tags during updates and surfaces DVLS response details.
- Safely handles failed authentication and bumps version to 1.3.1.
File summaries
| File | Description |
|---|---|
tests/integration/test_manage_server.yml |
Replaces debug output with server assertions. |
tests/integration/test_manage_secrets.yml |
Adds isolated secret lifecycle tests and cleanup. |
tests/integration/test_lookup_plugins.yml |
Reworks lookup tests around generated credentials. |
tests/integration/configurations.yml |
Uses the current run’s generated entry name. |
plugins/modules/fetch_server.py |
Prevents logout after failed login. |
plugins/modules/fetch_secrets.py |
Prevents logout after failed login. |
plugins/modules/create_secret.py |
Fixes payloads, tag preservation, errors, and logout handling. |
galaxy.yml |
Bumps the collection version to 1.3.1. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| app_key = module.params["app_key"] | ||
| app_secret = module.params["app_secret"] | ||
|
|
||
| token = None |
There was a problem hiding this comment.
Bien vu, requirements.txt épingle ansible~=11.3 donc la suite ne tournait que sur core 2.18, exactement la version où le bug est invisible.
Ajouté: tests/integration/test_error_handling.yml appelle les trois modules contre un serveur injoignable et vérifie que c'est bien l'erreur de login qui remonte, avec un job matriciel sur ansible 11 et ansible 12. Vérifié en réintroduisant le bug localement: sur 2.19 le test échoue avec « fetch_server masked the login error instead of reporting it: Module result deserialization failed ».
73090a3 to
f6bcd7d
Compare
Danny Bédard (DannyBedard)
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Integration tests can expose credential data in verbose logs and leave direct API sessions unreleased.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tests/integration/test_manage_secrets.yml:220
- The three direct API login tasks in this play never call
/api/v1/logout, and reassigningdvls_loginalso loses the earlier token. Each CI run can therefore leave multiple sessions active until server-side expiry; reuse the first token where possible and revoke every acquired token from analwayscleanup path, as the collection modules do.
- name: Log in to DVLS
ansible.builtin.uri:
url: "{{ lookup('env', 'DVLS_SERVER_BASE_URL') }}/api/v1/login"
method: POST
body_format: json
tests/integration/test_lookup_plugins.yml:103
- This cleanup creates a fresh DVLS session but never calls
/api/v1/logout, so repeated CI runs can accumulate active tokens until server-side expiry. Wrap deletion in a nested block/always and revokecleanup_login.json.tokenIdthrough the logout endpoint even when deletion fails.
- name: Log in to DVLS for the cleanup
ansible.builtin.uri:
url: "{{ lookup('env', 'DVLS_SERVER_BASE_URL') }}/api/v1/login"
method: POST
body_format: json
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
f6bcd7d to
b2e79d1
Compare
…eanup token The suite runs on ansible-core 2.18 only, so it could not catch the login failure masking that only shows up on 2.19. A new play calls every module against an unreachable server and asserts the login error is reported, and it runs on both ansible 11 and ansible 12. The update test now seeds a tag through the API and checks it survives, so it actually covers the tag preservation it relies on. The cleanup passed tokenId through the generic uri headers argument, which Ansible does not redact, so a verbose or failed cleanup could print it. Those tasks are no_log now.
b2e79d1 to
8170972
Compare
|
Deuxième passe traitée. Les tâches qui ramènent des credentials sont Sur les sessions API laissées ouvertes que mentionne le résumé: un logout explicite a été essayé puis retiré. Une app identity DVLS ne tient qu'une session à la fois, donc chaque login invalide le précédent et le play en garde au plus une, qui expire seule. La tâche de logout ajoutait surtout une surface d'échec dans le bloc de nettoyage. Reste connu et non traité ici: le test de lookup par path dépend encore de l'entrée Run vert: https://github.com/Devolutions/ansible-dvls/actions/runs/34521604516 |
La CI était rouge depuis avril: les tests dépendaient d'entrées déjà présentes sur le serveur, dont un id codé en dur, et faisaient surtout des
debugau lieu d'assertions. Chaque play crée maintenant ce qu'il lit sous un nom unique au run, vérifie ce qu'il a écrit, et supprime ses entrées ensuite. Deux vrais bugs sont sortis de là.create_secretne pouvait plus mettre à jour un secret existant: DVLS exige maintenant le champtagssur un PUT et renvoyait un 400 nu. Les mises à jour renvoient les tags que l'entrée possède déjà, et le test le vérifie en posant un tag puis en le relisant après coup.Tout échec de login remontait en « Module result deserialization failed » sur ansible-core 2.19, parce que le
finallyappelaitlogoutavec un token jamais assigné. C'est le cas de support de février, invisible sur 2.18. Un nouveau job rejoue les trois modules contre un serveur injoignable, sur ansible 11 et ansible 12, pour que ça ne repasse plus inaperçu.Les erreurs de DVLS sont aussi remontées telles quelles au lieu du seul code HTTP. C'est ce qui a permis de trouver les deux bugs.
Run vert: https://github.com/Devolutions/ansible-dvls/actions/runs/34516089755