Correctly update environment from the importing plan

[thrix]

The environment from the importing plan was ignored.

As agreed, let's make the environment a cached property and inherit
the importing plan environment automatically.

Resolves #2446

Pull Request Checklist

• implement the feature
• extend the test coverage

[thrix]

@happz @psss can you check if this would be the obvious fix? then I can write some tests.

Maybe some unit tests would be nice?

[LecrisUT]

Weird thing is that this is being tested:

tmt/tests/plan/import/modify-data/plans.fmf

Lines 8 to 16 in 0b95455

	/importing-other-plan-and-modify-environment:
	summary: Import plan and modify it with environment variable
	environment:
	VARIABLE: foobar
	plan:
	import:
	url: https://github.com/teemtee/tmt
	path: /tests/plan/import/modify-data
	name: /plans/must-be-imported-and-modified

[thrix]

Weird thing is that this is being tested:

tmt/tests/plan/import/modify-data/plans.fmf

Lines 8 to 16 in 0b95455

	/importing-other-plan-and-modify-environment:
	summary: Import plan and modify it with environment variable
	environment:
	VARIABLE: foobar
	plan:
	import:
	url: https://github.com/teemtee/tmt
	path: /tests/plan/import/modify-data
	name: /plans/must-be-imported-and-modified

Thank @LecrisUT looking into why it was not caught

[LecrisUT]

Oh, I have an idea. Could it be that when environment is treated inside defining the plan/test is different from when it is used for execute, i.e. if it's used as replacing a variable in .fmf file vs when used in executing a .sh file?

[psss]

Could it be that when environment is treated inside defining the plan/test is different from when it is used for execute, i.e. if it's used as replacing a variable in .fmf file vs when used in executing a .sh file?

Yes, seems that the explicit node data expansion handles correctly the case when variable is in the config:

tmt/tmt/base.py

Lines 2345 to 2348 in 722563c

	with tmt.utils.modify_environ(self.environment):
	self._expand_node_data(node.data, {
	key: ','.join(value)
	for (key, value) in self._fmf_context.items()})

But its value does not get to the actual environment when an external script is executed.

[psss]

@happz @psss can you check if this would be the obvious fix?

The fix seems to be ok. Although, when reading the code I realized that in a76772f we've introduced double-updating the environment from the command line. This does not hurt but let's get that straight: I've filed #2461 to track this.

then I can write some tests.

Yes, please cover the scenario when the variable is used in an external script, not directly in the fmf file.

[thrix]

@happz so I moved back from cached_property as mypy is not happy about it:

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

tmt/steps/__init__.py:1519: error: Cannot determine type of "discover"  [has-type]
tmt/steps/__init__.py:1598: error: Cannot determine type of "provision"  [has-type]
tmt/steps/__init__.py:1684: error: Cannot determine type of "execute"  [has-type]
tmt/steps/__init__.py:1713: error: Cannot determine type of "provision"  [has-type]
tmt/steps/report/__init__.py:95: error: Cannot determine type of "execute"  [has-type]
tmt/steps/discover/__init__.py:199: error: Cannot determine type of "execute"  [has-type]
tmt/steps/discover/__init__.py:201: error: Cannot determine type of "execute"  [has-type]
tmt/steps/discover/__init__.py:233: error: Cannot determine type of "execute"  [has-type]
Found 8 errors in 3 files (checked 59 source files)

[thrix]

@psss I am reading now your comment, I believe my test is enough, even though I am not sure why I added it to basic.sh, anyway, I do not mind it much to revisit this again.

Let me know if you want more proof it works as expected, I believe it does catch the problem, when I run it with old code, my test fails

[psss]

Yeah, looks good. Thanks for the fix. I just remembered we've agreed on the hacking session that we could use an environment property which could also cover the #2461 but let's handle that one separately.

[LecrisUT]

I've confirmed that the changes here work as intended

[psss]

/packit test --identifier full