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

OCM-5027 | feat: Added KubeletConfigClient and mock to be shared between Terraform and ROSA CLI #23

Conversation

robpblake
Copy link
Contributor

@robpblake robpblake commented Dec 6, 2023

See terraform-redhat/terraform-provider-rhcs#413

This PR comes from a discussion on the above PR with @nirarg . Given that the KubeletConfigClient interface, implementation, mock and matcher are not specific to the TF project, the proposal is to move these into ocm-common. This then provides a mechanism for us to re-use this client in both the ROSA CLI and Terraform.

The KubeletConfigClient has been designed to make it easier to work with the OCM GO SDK in unit tests by providing a clear interface and then mocks that can be used to setup and verify expected interactions. The goal here being to keep the tests light weight and only interact with the OCM SDK in e2e scenarios.

This provides a base for a pattern for us to extend as we look to make both TF and the ROSA CLI more unit testable.

@robpblake robpblake force-pushed the ocm-5027-common-kubeletclient branch from 521af2f to bbe40ac Compare December 6, 2023 15:35
// interacting with the OCM API
//
//go:generate mockgen -source=kubeletconfig_client.go -package=testing -destination=testing/mock_kubeletconfig_client.go
type KubeletConfigClient interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if there's a way we could extend this for subClusterResource more generically so we can share this behavior for ingress, machine pool etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took an attempt at this in #24

@nirarg
Copy link
Contributor

nirarg commented Dec 6, 2023

Thank you @robpblake
I think this is good start and this can be expanded in the future to all OCM resources
@ciaranRoche please take a look
/lgtm

@ciaranRoche
Copy link
Collaborator

Personally, I think it be better to go with - #24

@robpblake
Copy link
Contributor Author

Closing in favour of #24

@robpblake robpblake closed this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants