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

Refacter service secrets function #421

Conversation

bshephar
Copy link
Collaborator

This change refactors the generateServiceSecrets function to deduplicate code and move logic related to vhosts and templateParameters into dedicated functions.

@openshift-ci openshift-ci bot requested review from dprince and viroel September 16, 2024 06:33
@bshephar bshephar force-pushed the refactor-service-configs branch from 39fba0c to ee978b0 Compare September 16, 2024 11:20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know this adds a new style of testing. But I feel like Heat operator is particularly difficult to test because of the Gophercloud dependency. So we either mock the entire thing, or setup an API server to send OpenStack API like responses. Both of those options feel like a lot of work, so I propose that we instead try to make our code more natively testable by splitting up logic into smaller functions and then using table driven tests to validate their functionality.

In this particular case, the table driven test allowed for me to pass TLS and Non TLS scenarios quickly, while in envtest we just validate the resulting secret data for one off those scenarios. I think if we take this same style and logic and apply it to the rest of the repo, we can improve our test coverage here without the additional burden of the OpenStack API responses.

This change refactors the generateServiceSecrets function to deduplicate code and
move logic related to vhosts and templateParameters into dedicated
functions.

Signed-off-by: Brendan Shephard <[email protected]>
Signed-off-by: Brendan Shephard <[email protected]>
@bshephar
Copy link
Collaborator Author

Kuttl is failing due to the issue with the latest go-toolset in Go 1.21. We'll need to wait for that to be fixed to recheck on this one.

@stuggi
Copy link
Contributor

stuggi commented Sep 26, 2024

we've put in a custom image in place, the issue should be gone

@stuggi
Copy link
Contributor

stuggi commented Sep 26, 2024

/retest

@openshift-ci openshift-ci bot added the lgtm label Sep 26, 2024
Copy link
Contributor

openshift-ci bot commented Sep 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, fao89

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-bot openshift-merge-bot bot merged commit 95ef254 into openstack-k8s-operators:main Sep 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants