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

[cleanup] [client/k8s.go] Merge similar functionality into one decent function #32

Closed
vharsh opened this issue Jun 17, 2021 · 1 comment
Assignees
Labels

Comments

@vharsh
Copy link
Member

vharsh commented Jun 17, 2021

Problem statement

Currently the client/k8s.go file has nearly 530 LoC, a lot of which is just duplicated functionality or slightly similar functionality, e.g. GetCStorVolumeAttachmentMap(), GetCVA(string), etc.
There's a general trend in this file where-in, a resource has a wrapper to

  • GET a single resource of a particular KIND
  • LIST all the resources of a particular KIND
  • LIST all resource and return a map of them by some criteria, viz resource-name ➝ resource, or resource-label-key ➝ resource
    All of this is helpful in different use-cases, but it sort of pollutes the method this package is exposing and it'll likely keep increasing as support for more features are added, for example client package could have methods such as GetPV, GetPVs, GetPVMap, GetAllCStorPV. If a new resource is added(e.g. CV), the problem will get worse as now, there might be 4 more methods per resource(assuming same or similar requirements)

Potential solution(s)

Low Hanging fruit

  • It might help crunch the LoC by merging some of GET & LIST functionality into one method, e.g. GetPV(pvName string) (*corev1.PersistentVolume, error) & GetPV() (*corev1.PersistentVolumeList, error) & GetPVs(pvNames []string) (*corev1.PersistentVolumeList, error) into ONE ListPV(vols []string) (*corev1.PersistentVolumeList, error)
  • The newer one can perform a
    • GET, if the vols list has exactly one element,
    • A LIST if vols list is nil
    • A LIST & filter out the vols PVs if a set of one or more volumes are specified

NOTE: Need to double-check if there'd be any possible dead-ends with this refactor, specially during the fetch of namespaced resources.

@vharsh
Copy link
Member Author

vharsh commented Jun 29, 2021

Closing this, as this is done.

@vharsh vharsh closed this as completed Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants