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

[Task] Refactor eks.go #5482

Open
Himangini opened this issue Jul 4, 2022 · 6 comments
Open

[Task] Refactor eks.go #5482

Himangini opened this issue Jul 4, 2022 · 6 comments
Labels
area/tech-debt Leftover improvements in code, testing and building good first issue Good for newcomers priority/important-longterm Important over the long term, but may not be currently staffed and/or may require multiple releases

Comments

@Himangini
Copy link
Collaborator

Himangini commented Jul 4, 2022

Description

This ticket is to refactor the functionality mentioned below and here

eks.go

Contains methods for interacting with clusters. Seems good for this package ✔️

func (c *ClusterProvider) DescribeControlPlane(meta *api.ClusterMeta) (*awseks.Cluster, error)
func (c *ClusterProvider) RefreshClusterStatus(spec *api.ClusterConfig) error
func (c *ClusterProvider) SupportsManagedNodes(clusterConfig *api.ClusterConfig) (bool, error)
func (c *ClusterProvider) SupportsFargate(clusterConfig *api.ClusterConfig) (bool, error)
func (c *ClusterProvider) RefreshClusterStatusIfStale(spec *api.ClusterConfig) error
func (c *ClusterProvider) CanOperate(spec *api.ClusterConfig) (bool, error)
func (c *ClusterProvider) CanOperateWithRefresh(spec *api.ClusterConfig) (bool, error)
func (c *ClusterProvider) CanUpdate(spec *api.ClusterConfig) (bool, error)
func (c *ClusterProvider) ControlPlaneVersion() string
func (c *ClusterProvider) ControlPlaneVPCInfo() awseks.VpcConfigResponse
func (c *ClusterProvider) GetCluster(clusterName string) (*awseks.Cluster, error)
func (c *ClusterProvider) WaitForControlPlane(meta *api.ClusterMeta, clientSet *kubernetes.Clientset) error
func ClusterSupportsManagedNodes(cluster *awseks.Cluster) (bool, error)
func ClusterSupportsFargate(cluster *awseks.Cluster) (bool, error)
func PlatformVersion(platformVersion string) (int, error)

Also contains some stuff not-so related to EKS that should be moved ❎

func (c *ClusterProvider) NewOpenIDConnectManager(spec *api.ClusterConfig) (*iamoidc.OpenIDConnectManager, error)
func (c *ClusterProvider) LoadClusterIntoSpecFromStack(spec *api.ClusterConfig, stackManager manager.StackManager) error
func (c *ClusterProvider) LoadClusterVPC(spec *api.ClusterConfig, stackManager manager.StackManager) error
@Himangini Himangini added the area/tech-debt Leftover improvements in code, testing and building label Jul 4, 2022
@Himangini Himangini added good first issue Good for newcomers priority/important-longterm Important over the long term, but may not be currently staffed and/or may require multiple releases labels Jul 22, 2022
@shogohida
Copy link

@Himangini
Can I work on this?

@Himangini
Copy link
Collaborator Author

@Himangini Can I work on this?

@shogohida we always encourage community contributions 🎉 please feel free to pick up, we're are happy to help with your PRs ✨

@shogohida
Copy link

@Himangini
Thanks for your comment! I have a question.

Should I move the functions below to pkg/aws/eks.go?

func (c *ClusterProvider) NewOpenIDConnectManager(spec *api.ClusterConfig) (*iamoidc.OpenIDConnectManager, error)
func (c *ClusterProvider) LoadClusterIntoSpecFromStack(spec *api.ClusterConfig, stackManager manager.StackManager) error
func (c *ClusterProvider) LoadClusterVPC(spec *api.ClusterConfig, stackManager manager.StackManager) error

It seems the explanation here indicates so but I wanted to make sure.

スクリーンショット 2022-10-20 1 45 17

@shogohida
Copy link

Is my understanding correct?

#5810

@Himangini
Copy link
Collaborator Author

Is my understanding correct?

#5810

As mentioned in the description, the functions need to be moved to a relevant place. if you are happy with your changes please open the PR for review 👍🏻 we are working over a shorter capacity right now but will get your contribution reviewed soon.

@shogohida
Copy link

@Himangini
Understood. I opened the PR so please review it when you have time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tech-debt Leftover improvements in code, testing and building good first issue Good for newcomers priority/important-longterm Important over the long term, but may not be currently staffed and/or may require multiple releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants