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

Fix command ID #495

Merged
merged 1 commit into from
Nov 16, 2019
Merged

Fix command ID #495

merged 1 commit into from
Nov 16, 2019

Conversation

aelsabbahy
Copy link
Member

@sshipway Since you were the original author of the command/exec change, I would love your feedback on this, since I don't know the rationale behind the command.ID change.

This change changes the output of the test result, so that this test file:

command:
  python_example:
    exec: |
      python3 << EOF
      import sys
      print('example output')
      for i in range(3):
        print(i)
      sys.exit(15)
      EOF
    stdout:
      - 'example'
      - '0'
      - '1'
      - '2'
    exit-status: 15
    timeout: 10000

Current state:

$ goss v --format documentation
Command: python_example: python3 << EOF
import sys
print('example output')
for i in range(3):
  print(i)
sys.exit(15)
EOF
: exit-status: matches expectation: [15]
Command: python_example: python3 << EOF
import sys
print('example output')
for i in range(3):
  print(i)
sys.exit(15)
EOF
: stdout: matches expectation: [example 0 1 2]


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

With this PR:

$ goss v --format documentation
Command: python_example: exit-status: matches expectation: [15]
Command: python_example: stdout: matches expectation: [example 0 1 2]


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

@sshipway
Copy link
Contributor

sshipway commented Nov 13, 2019

The rationale behind the change to the ID function was so that the logs would continue to show the command being run, as they did before the change.

However, since I had not considered that now it is possible to hide an entire script in the exec attribute, it is clear that the resulting behaviour is not optimal. So, I agree that this change (just displaying the key, even if an exec attribute is defined) is the best way to go.

So - thanks for asking, and I agree this is a good idea

@aelsabbahy aelsabbahy merged commit 4aa1330 into master Nov 16, 2019
BenjaminHerbert pushed a commit to BenjaminHerbert/goss that referenced this pull request May 28, 2020
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.

2 participants