Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: create e2e resources from yaml files #237

Merged
merged 1 commit into from
Aug 17, 2022
Merged

fix: create e2e resources from yaml files #237

merged 1 commit into from
Aug 17, 2022

Conversation

nbalacha
Copy link
Contributor

Modified the e2e code to use go:embed and yaml files
to create pod,pvc,and volumesnapshot resources. This makes
it easier to make changes when required.

Signed-off-by: N Balachandran [email protected]

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2022
@nbalacha
Copy link
Contributor Author

/retest-required

@nbalacha
Copy link
Contributor Author

/retest

size = "5Gi"
)
//go:embed testdata/pvc_tests/pvc-template.yaml
var pvcYAMLTemplate string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAML can be removed from each of the variables. Looks redundant IMO. pvcTemplate should suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the yaml makes it a little clearer as to what type it is in the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

if err != nil {
return nil, err
}
pvc := obj.(*k8sv1.PersistentVolumeClaim)
Copy link
Contributor

@sp98 sp98 Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think obj can returned here without casting it to actual resource because the return type of all these decoder functions is used in the crClient.Create function that can take the obj.
So we can have a single function that just decodes all the input yaml strings to obj.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer casting it because it makes the code clearer as to what object it is working on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

volumeSnapshotClassName: odf-lvm-vg1
source:
persistentVolumeClaimName: lvmfilepvc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line.

By("Creating a clone of the block-pvc")
clonePvc = getSamplePvc(size, pvc.Name+"-clone", k8sv1.PersistentVolumeBlock, storageClass, "PersistentVolumeClaim", pvc.Name)
pvcCloneYaml := fmt.Sprintf(pvcCloneYAMLTemplate, "lvmblockpvc-clone", testNamespace, "Block", storageClass, "lvmblockpvc")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we rather create multiple template files for reach resource type rather than updating the template like this?
I think the main purpose of the go embed would be to use static assets that don't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is necessary - we can use the same template to create multiple instances of a type.
Having a separate file for each instance could quickly get out of hand once the tests are expanded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Modified the e2e code to use go:embed and yaml files
to create pod,pvc,and volumesnapshot resources. This makes
it easier to make changes when required

Signed-off-by: N Balachandran <[email protected]>
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nbalacha, sp98

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 848d21b into openshift:main Aug 17, 2022
@nbalacha nbalacha deleted the embed-e2e branch September 6, 2022 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants