test rex using values from global parameters #17043
Conversation
pondrejk
added
Stream
| for key_val in key_vals: | ||
| param, new_value = tuple(key_val.split('=')) if '=' in key_val else (key_val, None) | ||
| existing_params = target_sat.api.CommonParameter().search(query={'search': f'name={param}'}) | ||
| if len(existing_params) > 0: |
There was a problem hiding this comment.
Can this be >1? Should it? Should we assert <=1?
There was a problem hiding this comment.
It shouldn't be, if yes that would be a bug. I can add an assert here, it would do no harm, though I see there is some sort of coverage for this case here
|
|
||
|
|
||
| @pytest.fixture | ||
| def multi_setting_update(request, target_sat): |
There was a problem hiding this comment.
Have you decided to keep the old fixture, even though this is more generalized, for backwards compatibility?
There was a problem hiding this comment.
This I created as a first version thinking I'll need to update settings, I don't use it in the test but I decided to keep it if someone needs it in the future. Could be removed though if it causes confusion
There was a problem hiding this comment.
To rephrase my question: why did you keep fixture setting_update?
There was a problem hiding this comment.
ok I see, mainly because that one is already used at a number of places, I suppose I'd need to change all these fixture calls so that they supply list instead of a single item
| @@ -311,6 +311,96 @@ def test_positive_run_job_effective_user( | |||
| } | |||
| ) | |||
|
|
|||
| @pytest.mark.tier3 | |||
| @pytest.mark.parametrize( | |||
| 'multi_global_param_update', | |||
There was a problem hiding this comment.
To self-answer the question I've had: why use global parameters when there are global settings just for this? I think global parameters are used in templating (to setup execution) and global settings are used to actually execute.
There was a problem hiding this comment.
It is a bit messy, from what I got, some settings are mirrored in global params, but for many cases, the global params are the only way to go (this case), and for other cases, there are just settings...
There was a problem hiding this comment.
more on your question, I need global params to exercise the templating part for SAT-28443, it's the part after line 376:
# re-register host to run the remote_execution_ssh_keys snippet with new defaults
client.register(module_org, None, module_ak_with_cv.name, module_target_sat, force=True)
# check the sudoers.d entry was created by the snippet
result = client.execute(f'''stat -c "%a %n" /etc/sudoers.d/{ssh_username}''')
assert '440' in result.stdout
but it is true that the rest of the test could be served by settings, I may go on and create a separate test for it
| 'multi_global_param_update', | ||
| [ | ||
| [ | ||
| 'remote_execution_effective_user', |
There was a problem hiding this comment.
param, new_value = tuple(key_val.split('=')) if '=' in key_val else (key_val, None)
So here, you save the parameters and restore them later in cleanup. Only to later set them to some value in the test itself. Isn't it better to just set them to desired value here? Or, if you need randomness, in a separate fixture?
There was a problem hiding this comment.
the idea is to return to the original state after test is run. I set it in test for randomness, I'd say a separate fixture would be a bit of an overkill
| param_object.value = new_value | ||
| param_object.update({'value'}) | ||
| else: | ||
| param_object = target_sat.api.CommonParameter(name=param, value=new_value).create() |
There was a problem hiding this comment.
Should we check for new_value is not None here?
There was a problem hiding this comment.
on the second thought, that check for None doesn't really make sense if we admit that params could be set to None
| else: | ||
| param_object = target_sat.api.CommonParameter(name=param, value=new_value).create() | ||
| cleanup = True | ||
| default_param_value = new_value |
There was a problem hiding this comment.
Why? We do cleanup anyway.
There was a problem hiding this comment.
not the most elegant I admit, but I need to create that var as I append the object outside of that if/else structure on the next line
Problem Statement
mostly to cover SAT-28443, but also to check global params usage
Solution
note: It turned out the scenario needs global params and not global settings, but since I already wrote the multi_setting_update fixture, I left it there for potential future use
also stream has hickups with AKs, so holding of prt for now
Related Issues
SAT-28443