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

(testing): Update testdata generation and testing logic #4

Merged

Conversation

everettraven
Copy link
Collaborator

@everettraven everettraven commented Aug 4, 2023

Description of the change:

  • Updates testdata generation to use a new library implemented in this PR for scaffolding out a phase 1.5 plugin without building it into a build of the operator-sdk or kubebuilder.
  • Generates new testdata
  • Updates e2e tests to generate new testdata on the fly and use the helpers from the new library added
  • Added a new library to help with generating testdata for phase 1.5 plugins and generalize some of the commonly used logic for testing the sample operators.

Motivation for the change:

  • Update the testdata generation and e2e testing logic to ensure we are properly testing the phase 1.5 plugin implementation

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2023
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven everettraven marked this pull request as ready for review August 14, 2023 17:40
@everettraven everettraven changed the title WIP: (testing): Update testdata generation and testing logic (testing): Update testdata generation and testing logic Aug 14, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2023
)

// InstallCertManagerBundle installs CertManager onto a Kubernetes cluster
func InstallCertManagerBundle(hasv1beta1CRs bool, kubectl kubernetes.Kubectl) error {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the legacy versions what support v1beta1 CRDs? I believe we need not even test again such old k8s cluster versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good point. I think a lot of this was copied over from a previous PR against the operator-sdk that, at the time, did test v1beta1 CRDs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated this to no longer have a bool for if there are v1beta1 CRs, but left the legacy versions in place for now. I'm happy to remove the legacy version support all together but felt that warranted at least a bit of conversation (although I do agree it's unlikely we will be testing against kube <= 1.16 so I'm +1 on removing)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After a sync meeting it was decided to eventually try and push these changes upstream to Kubebuilder. With that in mind I would lean towards keeping this as is for now and making further improvements when contributing this to Kubebuilder.

pkg/testutils/e2e/olm/helpers.go Outdated Show resolved Hide resolved
hack/generate/samples/ansible/generate.go Show resolved Hide resolved
pkg/testutils/e2e/certmanager/helpers.go Show resolved Hide resolved

// AllowProjectBeMultiGroup will update the PROJECT file with the information to allow we scaffold
// apis with different groups. be available.
func AllowProjectBeMultiGroup(sample sample.Sample) error {
Copy link
Member

Choose a reason for hiding this comment

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

we should be having a separate test/scaffolding for multi group. Can't we set this in the init command itself?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The advanced molecule testdata sample enables multi-group support. I don't see why we couldn't set this in the init command itself, but I don't recall if the sample generator currently allows for setting custom flags in the init subcommand at the moment.

pkg/testutils/e2e/kind/helpers.go Show resolved Hide resolved
pkg/testutils/e2e/olm/helpers.go Outdated Show resolved Hide resolved
pkg/testutils/sample/cli/cli.go Outdated Show resolved Hide resolved
pkg/testutils/kubernetes/kubectl.go Show resolved Hide resolved
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven everettraven mentioned this pull request Aug 17, 2023
2 tasks
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

Did a quick review and it looks good. I'm not looking in detail at the test library implementation, have a view suggestions on it, but we can look at it later. No need to block this PR for that!

Nice work! :)
/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@everettraven everettraven merged commit 1d7b885 into operator-framework:main Aug 22, 2023
4 checks passed
@everettraven everettraven deleted the test/update-testing branch August 22, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants