Allow setting order and hooks for test role #2374
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks for the PR! ❤️ |
eshulman2
force-pushed
the
test_operator_stages
branch
from
d749fd6
to
9e7ab2d
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e1ceb9f2cf3d47658a3de25c1ed70de8 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 39m 16s |
eshulman2
force-pushed
the
test_operator_stages
branch
3 times, most recently
from
f24ddba
to
c4de17d
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/a25d6668f88443eeb5b2e354691eb8dc ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 39m 51s |
eshulman2
force-pushed
the
test_operator_stages
branch
2 times, most recently
from
c175381
to
5efbd8f
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ea84a8ceb689406f85a63f9e9f8d1718 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 39m 40s |
eshulman2
force-pushed
the
test_operator_stages
branch
3 times, most recently
from
1c61cd8
to
34ffac9
Compare
eshulman2
force-pushed
the
test_operator_stages
branch
2 times, most recently
from
c00804c
to
e6b67aa
Compare
eshulman2
requested review from
lpiwowar,
lewisdenny and
cjeanner
and removed request for
lpiwowar and
lewisdenny
eshulman2
force-pushed
the
test_operator_stages
branch
from
e6b67aa
to
d2b1fed
Compare
eshulman2
force-pushed
the
test_operator_stages
branch
6 times, most recently
from
e78e568
to
3504fa0
Compare
There was a problem hiding this comment.
I've added a few comments. Upstream jobs need to be updated as well (at least the one in the test-operator repo) [1].
@lpiwowar AFAIK the hooks that are part of this project are here https://github.com/openstack-k8s-operators/ci-framework/tree/main/hooks but as the hook can call any playbook from any location/repo/playbook I don't think this is something we can monitor 100% as you said but even without this patch people can still work around it. Maybe we can suggest people having a specific dir for d/s as well.
Yes, it seems that we are not going to be able to have full control of this. If we could by default read from the directory (or another one located within the test-operator role dir) you mentioned I think it wouldn't hurt and give a hint to the users of this role that any hooks should be located there. We would just hit the issue of downstream vs upstream hooks. Ideally all the hooks would be moved to upstream but this would take extra time.
[1] here
roles/test_operator/README.md
Outdated
| * `cifmw_test_operator_stages`: (List) List of dictionaries defining the stages that should be used in the test operator role. List items options are: | ||
| * `name`: (String) The name of the stage. The name must be unique. | ||
| * `type`: (String) The framework name you would like to call, currently the options are: tempest, ansibletest, horizontest, tobiko. | ||
| * `test_file`: (String) Path to the file used for testing, this file should contain the testing params for this stage. Default value: `{{ ansible_user_dir }}/ci-framework-data/artifacts/ansible-vars.yml` |
There was a problem hiding this comment.
What about test_vars_file or test_vars? The current name gives me a feeling that some actual testing is happening inside this file whereas it is a file with testing parameters.
Also, what about changing this interface so that the vars can be specified directly in the test_vars field?
cifmw_test_operator_stages:
- name: "stage-1-tempest"
type: "tempest"
test_vars:
cifmw_test_operator_abc: x
cifmw_test_operator_def: y
There was a problem hiding this comment.
@lpiwowar the reason I wanted it to be a file was to be able to avoid having to specify all the parameters inside the stages structure which can make it very large and hard to manage. I can add an additional interface if you think appropriate but I would prefer to keep the test var file.
There was a problem hiding this comment.
In the provided link most of the space of the stage definition is taken by the hooks. Looking at the stage definition I want to see what tests are being executed there and with what configuration. To me this starts to feel too nested. Job definition -> vars file -> tests file. This would mean that for each stage there will be with high probability a new test file. I'm not sure whether we want to go this way.
I would prefer to go with test_vars. I guess we need somebody to decide what is best.
But one way or the other I would rename the var to test_vars or test_vars_file.
There was a problem hiding this comment.
@lpiwowar please note that assuming you want to run several tempest stages you would have to specify ALL (different) variables in the stages definition. maybe it wasn't clear in the reference I shared but it runs completely different tempest stages 3 times having to specify all the vars in the stage definition would make it hard to manage. I believe having test variables in different files should simplify it and make it easier to manage and review. I don't mint having both but not having the file one would complicate things for some jobs IMHO.
There was a problem hiding this comment.
@eshulman2, ok, it make sense. In that case if everyone agrees, can we do these two things?
- Add a parameter
test_varswhich can be used in cases when the test definition is not lengthy. - Rename the
test_fileparameter totest_vars_file.
eshulman2
force-pushed
the
test_operator_stages
branch
from
3504fa0
to
e86feca
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f3e535a33c0d4119ad58509c98bac5dd ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 10m 52s |
eshulman2
force-pushed
the
test_operator_stages
branch
from
e86feca
to
09c6d8d
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/fd36ee025af1498aaf2f4438b661f2ff ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 08m 48s |
eshulman2
force-pushed
the
test_operator_stages
branch
from
09c6d8d
to
998733f
Compare
roles/test_operator/README.md
Outdated
| * `cifmw_test_operator_stages`: (List) List of dictionaries defining the stages that should be used in the test operator role. List items options are: | ||
| * `name`: (String) The name of the stage. The name must be unique. | ||
| * `type`: (String) The framework name you would like to call, currently the options are: tempest, ansibletest, horizontest, tobiko. | ||
| * `test_file`: (String) Path to the file used for testing, this file should contain the testing params for this stage. Default value: `{{ ansible_user_dir }}/ci-framework-data/artifacts/ansible-vars.yml` |
There was a problem hiding this comment.
@eshulman2, ok, it make sense. In that case if everyone agrees, can we do these two things?
- Add a parameter
test_varswhich can be used in cases when the test definition is not lengthy. - Rename the
test_fileparameter totest_vars_file.
eshulman2
force-pushed
the
test_operator_stages
branch
from
998733f
to
75165c4
Compare
eshulman2
force-pushed
the
test_operator_stages
branch
from
75165c4
to
d9db875
Compare
NOTE: this patch will require setting order for all jobs running not only tempest - Allow passing hook list directly to hook role - Allow setting up stages and hooks for test operator role Jira: https://issues.redhat.com/browse/OSPRH-10106
eshulman2
force-pushed
the
test_operator_stages
branch
from
d9db875
to
bba3b16
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/d5cbb3aea2394f379e2d7c11fe085007 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 01m 51s |
The linked PR for the test-oprator role introduces the concept of stages. This patch updates the job definition so that it is consuming the newly defined interface. It is required because by default only the tempest stage is enabled. Depends-On: openstack-k8s-operators/ci-framework#2374
There was a problem hiding this comment.
I have no major problems with the current state of this PR except for the one question raised in the README. Although, we should properly test it in downstream / upstream jobs and ensure smooth transition before we merge this.
I'm testing the PR here.
| * `name`: (String) The name of the stage. The name must be unique. | ||
| * `type`: (String) The framework name you would like to call, currently the options are: tempest, ansibletest, horizontest, tobiko. | ||
| * `test_vars_file`: (String) Path to the file used for testing, this file should contain the testing params for this stage. | ||
| * `test_vars`: (String) Testing params for this specific stage if a `test_vars_file` is used the specified parameters would override the ones in the `test_vars_file`. |
There was a problem hiding this comment.
I think overwriting is good 👍
What will happen if we have a job that has the following definition?
...
vars:
cifmw_test_operator_tempest_include_list: test1
cifmw_test_operator_stages:
- name: tempest
type: tempest
test_vars:
cifmw_test_operator_tempest_include_list: test2
If I am not mistaken, then test1 will be executed, right? This is IMO a little bit confusing. The precedence of the variables would be as follows:
global_vars > test_vars > file_vars
I would prefer the more "local" the variable is, the higher power of overwriting it has:
test_vars > file_vars > global_vars
Even if we omit test_vars then I think that test_vars_file should have higher precedence than global_vars (by global_vars I mean those defined in the first level of vars: section or those defined in a parent job). Am I missing something?
| - name: "Call runner {{ _stage_vars.type }}" | ||
| vars: | ||
| file_vars: "{{ _stage_var_file | default({}) }}" | ||
| test_vars: "{{ _stage_vars.test_vars | default({}) }}" |
There was a problem hiding this comment.
nit
| test_vars: "{{ _stage_vars.test_vars | default({}) }}" | |
| test_vars: "{{ _stage_vars.test_vars | default({}) }}" |
+1 from my side, we have a thread open d/s to have this patch tested. Pending that I'm happy with the code. |
Jira: https://issues.redhat.com/browse/OSPRH-10106