-
Notifications
You must be signed in to change notification settings - Fork 122
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
Add a lint
check for empty and missing environment files
#3204
base: main
Are you sure you want to change the base?
Conversation
6819c26
to
e1db5ce
Compare
ef711a8
to
c2b295a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving this! Added a couple comments. Would be good to handle the non-existent file as part of the lint
report instead of bailing out with a catched exception.
Would you mind adding a simple test coverage? You could just extend the existing test. Also, what about adding a short release note?
tmt/base.py
Outdated
""" P001: env files are not empty """ | ||
|
||
env_files = self.node.get("environment-file") or [] | ||
if env_files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the condition needed? What about dropping it altogether? The for
cycle would handle an empty list
itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gives a better message: when the list is empty, you get an immediate PASS
, while with a loop it would be hard to report such a pass as the loop would not indicate the number of iterations it took.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. If the list is empty, the for
loop would be skipped and you get right to the pass
statement. But yes, if the return
statement is misplaced, then it's a completely different story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, return
is misplaced. I suppose it's structured as other lint checks, e.g.:
def lint_unknown_keys(self) -> LinterReturn:
""" T001: all keys are known """
# We don't want adjust in show/export so it is not yet in Test._keys
invalid_keys = self._lint_keys(EXTRA_TEST_KEYS + OBSOLETED_TEST_KEYS + ['adjust'])
if invalid_keys:
for key in invalid_keys:
yield LinterOutcome.FAIL, f'unknown key "{key}" is used'
return
yield LinterOutcome.PASS, 'correct keys are used'
Without the if invalid_keys
, code would run the loop, finish the loop, and continue to yield PASS
, which would be fine when invalid_keys
is empty, but incorrect when it's not empty. Hence the test & hard return
in that branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PASS
statement, as suggested above, is missing. Thus you would never get ok:
> tmt lint
/plans/example
pass C000 fmf node passes schema validation
pass C001 summary key is set and is reasonably long
pass P001 correct keys are used
pass P002 execute step defined with "how"
pass P003 execute step methods are all known
skip P004 discover step is not defined
skip P005 no remote fmf ids defined
pass P006 phases have unique names
pass P007 execute phase 'default-0' does not require specific guest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated !
tmt/base.py
Outdated
env_file = Path(env_file).resolve() | ||
|
||
if not env_file.exists() or not env_file.stat().st_size: | ||
yield LinterOutcome.FAIL, f'the file "{env_file}" does not exists or is empty' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-existent file is actually not covered:
tmt lint
File '/tmp/empty/environment' doesn't exist.
We do not get the full lint
output as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, incorrectly indented return
, should be on the level of for
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, moving return
makes sense. It does not fix the problem mentioned above though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-existent case is still not fixed:
> tmt lint
File '/tmp/empty/environment' doesn't exist.
lint
check for empty and missing environment files
c2b295a
to
1db47af
Compare
tmt/base.py
Outdated
env_files = self.node.get("environment-file") or [] | ||
|
||
if not env_files: | ||
yield LinterOutcome.SKIP, 'no empty environment files found' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yield LinterOutcome.SKIP, 'no empty environment files found' | |
yield LinterOutcome.SKIP, 'no environment files found' |
Perhaps like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated !
@mcasquer, what do you think about adding a simple test and a short release note? |
Perfect, I will add them along with the above suggestions ! |
fbf2a76
to
22a3061
Compare
62aa444
to
3cd24be
Compare
New function that raises a failure if the size of the environment file, if this one exists, is zero. Signed-off-by: mcasquer <[email protected]>
3cd24be
to
b5890c4
Compare
New function that raises a failure if the size of
the environment file, if this one exists, is zero.
Pull Request Checklist