Add a lint check for empty and missing environment files

[mcasquer]

New function that raises a failure if the size of
the environment file, if this one exists, is zero.

Pull Request Checklist

• implement the feature
• extend the test coverage
• include a release note

[psss]

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?

[psss]

Is the condition needed? What about dropping it altogether? The for cycle would handle an empty list itself.

[happz]

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.

[psss]

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.

[happz]

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.

[mcasquer]

Updated !

[psss]

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

[mcasquer]

Updated !

[psss]

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.

[happz]

Meh, incorrectly indented return, should be on the level of for.

[psss]

I see, moving return makes sense. It does not fix the problem mentioned above though.

[psss]

The non-existent case is still not fixed:

> tmt lint

File '/tmp/empty/environment' doesn't exist.

[psss]

Suggested change

	yield LinterOutcome.SKIP, 'no empty environment files found'
	yield LinterOutcome.SKIP, 'no environment files found'

Perhaps like this?

[mcasquer]

Updated !

[psss]

Would you mind adding a simple test coverage? You could just extend the existing test. Also, what about adding a short release note?

@mcasquer, what do you think about adding a simple test and a short release note?

[mcasquer]

Would you mind adding a simple test coverage? You could just extend the existing test. Also, what about adding a short release note?

@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 !