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.

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

Added: warning when duplicate keys cause tests to be overwritten #843

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

aelsabbahy
Copy link
Member

@aelsabbahy aelsabbahy commented Sep 16, 2023

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description of change

Prints out warning messages when overlapping keys are detected, for example:

goss.yaml:

command:
  example_test:
    exec: echo "hi"
    exit-status: 0
    stdout:
    - hi
    stderr: ""
    timeout: 10000
gossfile:
  goss_embed.yaml: {}

goss_embed.yaml:

command:
  example_test:
    exec: echo "bye"
    exit-status: 0
    stdout:
    - bye
    timeout: 10000
$ goss -l warn v
2023-09-16T20:36:48Z [WARN] Duplicate key detected: 'command: example_test'. The value from a later-loaded goss file has overwritten the previous value.
..

Total Duration: 0.002s
Count: 2, Failed: 0, Skipped: 0

Same with render:

$ goss -l warn render > /dev/null
2023/09/16 13:37:07 [WARN] Duplicate key detected: 'command: example_test'. The value from a later-loaded goss file has overwritten the previous value.

This warning is printed for both failing and passing tests, essentially it is informing the user that the tests were overwritten.. which may or may not be desired.

As a consequence of this change, I moved some existing log levels around to match what I would expect as a user:

  • TRACE - Detailed internal system activities useful for goss developers to debug.
  • DEBUG - Information useful for the goss user to debug.
  • INFO - General operational messages, useful for tasks where a more structured output is needed (e.g. goss serve).
  • WARN - Non-critical issues that may require attention, such as overwritten keys or deprecated features.

closes #743

Questions

  1. Given the above classification, should the default log level be set to INFO? Users who find it noisy can do -l warn or -l error

@aelsabbahy
Copy link
Member Author

CC: @obourdon, @ripienaar,@plannigan, @GhuulCZ

Since you all have been involved with discussions around this.

@plannigan
Copy link

Seems reasonable to me. 👍

My preference would have been to actually error, this is much better than silently overwriting.

@aelsabbahy
Copy link
Member Author

@plannigan how about a layered approach

  1. Default, user gets a warning if they're not aware of this behavior. (This pr)
  2. User can pass --log-level ERROR to silence warnings.. perhaps they're intentionally using overrides. (This pr)
  3. User wants to explicitly block this behavior, they pass --strict (new pr)

@plannigan
Copy link

Yeah, that makes sense. Default to showing the warning. People can silence it if they want. People can also elevate it to an error if they want.

@aelsabbahy aelsabbahy merged commit ed3be13 into master Sep 20, 2023
@aelsabbahy aelsabbahy deleted the add_warnings_on_duplicate_keys branch September 20, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

render silently drops overlapping directives
2 participants