unioslo / mreg-cli

Command Line Interface for Mreg
GNU General Public License v3.0
2 stars 7 forks source link

Add interactive diff reviewing #270

Closed pederhan closed 2 months ago

pederhan commented 2 months ago

This PR adds interactive reviewing of diff changes.

image

Each accepted change is added to testsuite-result.json, eliminating the need to rename the test suite files and making it easier to incorporate partial changes.

Considerations

The PR has a somewhat breaking change in that it writes the new result with all timestamps, IPs, etc. replaced. If we want to write the actual results (i.e without IPs and timestamps replaced) we have to revise this PR considerably.

Running this PR on the existing testsuite-results.json file the first time and selecting "no" to all changes will still modify the file by replacing the aforementioned items.

terjekv commented 2 months ago

Timestamps are ephemeral, they can happily die in a fire. IPs though... I can see situations where the user expects a consistent behaviour when asking for an IP in a specific way. Ie, if we ask for a specific IP for a host, we should know that tis is the IP we get back. But, well, that is tested by the CLI now, so the output isn't a validator for that per se? Hm.

pederhan commented 2 months ago

Getting some different results locally vs in CI:

Local:

ERROR: 224 unresolved diff(s) between testsuite-result.json and new_testsuite_log.json.

CI:

ERROR: 206 unresolved diff(s) between testsuite-result.json and new_testsuite_log.json.

Investigating...

I need to find out why the CI diff mentions this, which I can't replicate locally:

{                                                                           
      "command": "zone set_default_ttl example.org 60",                         
      "command_filter": null,                                                   
      "command_filter_negate": false,                                           
      "command_issued": "zone set_default_ttl example.org 60 # should fail, mu  
      "ok": [],                                                                 
      "warning": [                                                              
  -     "Invalid TTL value: 60 (300->68400)"                                    
  ?                                 ^                                           
  +     "Invalid TTL value: 60 (300->68400)"                                 
  ?                                 ^^^^                                        

Aha, it's because we now escape error messages, which I implemented in #269... FWIW, it renders normally in the console, it just affects the messages we record:

super@mreg-dev.uio.no> zone set_default_ttl pederhan.uio.no 60
Invalid TTL value: 60 (300->68400)
terjekv commented 2 months ago

Aha, it's because we now escape error messages, which I implemented in #269... FWIW, it renders normally in the console, it just affects the messages we record:

Yeah, that will work after the first time, but it makes the recorded data hard to read. :(

pederhan commented 2 months ago

Yeah, that will work after the first time, but it makes the recorded data hard to read. :(

https://github.com/unioslo/mreg-cli/pull/271