fix: More YAML templates for E2E virt tests #1364
Conversation
Signed-off-by: Mateus Oliveira <[email protected]>
openshift-ci
bot
added
the
approved
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
There was a problem hiding this comment.
I like having this code simplified into YAML templates, but I know @weshayutin is really in favor of having plain YAML files. I am wondering if a good compromise is to move the infrastructure-related templates (the HCO installation, KVM emulation, and maybe a new one for the latest CirrOS image fetching) out of sample-applications into a separate directory under tests/e2e, and renamed to something like *.yaml.template. Then the actual VMs themselves can be plain .yaml files under sample-applications like they are now, with no templating.
Otherwise, @mpryc had an interesting idea about parsing the sample YAML into actual CRD structs and replacing the relevant fields That way, there could be fixed YAML files in the current directory, and each test could decide to override any fields as needed. Maybe with a command line argument or environment variable to enable or disable this replacement, to force the test to respect any manually-added YAML changes.
Do either of these ideas sound useful?
|
I would say the following.. How the templates are constructed is NOT a big deal to me unless the templates are used in documentation in oadp-operator/docs. I am happy with working templates and we shouldn't bike shed too much about it. YAML templates are fine, we should have a little shell script that can populate and render the templates. Breaking out files into directories that make more sense is also fine. Re the sample apps in our tests/e2e
|
| template, err := os.ReadFile(file) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| obj := &unstructured.UnstructuredList{} | ||
|
|
||
| // YAML templates can have replace directives, in the form of "<<template-replace-N>>", where N |
There was a problem hiding this comment.
Is this oadp specific directive or we're following some published standard?
There was a problem hiding this comment.
just from my head 🤣
tried to think what would be good in the future using this
I accept suggestions
|
@mrnold did not understand how would be @mpryc idea we can document yes @weshayutin . This can be very useful for me in the future. Today (in IBM) I need to manually edit all snapclasses (because I do not use ceph), for example. This could be automated with templates (and not change how other people run But seems we have some discussion, so we can focus on working here after 1.3.1 (unless @mrnold wants anything from here now) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mateusoliveira43, mrnold The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@mateusoliveira43: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
@mateusoliveira43 thank you for putting up this change, it fell off my radar tbh. I have strong opinions about this that should be tempered by a group discussion. I'll raise it in thrs scrum for discussion. I'd like Matthew to review in depth but he is focused on dataupload/datadown changes, this will have to wait a bit. |
|
Created issue to discuss solution #1531 |
Why the changes were made
Follow up for #1359 to add more YAML templates.
Blocked by #1386How the changes were made
Studied code to find and remove "YAML literals" from it. Added feature to replace strings in code in templates.
How to test the changes made
If you have 4.14+ cluster, you can run E2E virt tests locally with
make TEST_VIRT=true test-e2eCheck logs, they should not be less informative and readable as they were before this change.