-
Notifications
You must be signed in to change notification settings - Fork 2
Split up server.go into diff files #472
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 !
Looking great
return &capiv1_proto.GetEnterpriseVersionResponse{ | ||
Version: version.Version, | ||
}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maye its own file called version
?
// profileValues is what the client will provide to the API. | ||
// It may have > 1 and its values parameter may be empty. | ||
// Assumption: each profile should have a values.yaml that we can treat as the default. | ||
func generateProfileFiles(ctx context.Context, helmRepoName, helmRepoNamespace, helmRepositoryCacheDir, clusterName string, kubeClient client.Client, profileValues []*capiv1_proto.ProfileValues) (*gitprovider.CommitFile, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpers that are only used by the clusters handlers can go in that file I would say, wdyt?
The principle of proximity focuses on how well organised your code is with respect to readability and change. Proximity implies that functions that are changed together are moved closer together. Proximity is both a design principle and a heuristic for refactoring hotspots toward code that's easier to understand.
^ Not a hard and fast rule at all just guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me 🙂 🙏
repositoryURL := os.Getenv("CAPI_TEMPLATES_REPOSITORY_URL") | ||
|
||
return &capiv1_proto.GetConfigResponse{RepositoryURL: repositoryURL}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.go?
} | ||
|
||
// ListCredentials searches the management cluster and lists any objects that match specific given types | ||
func (s *server) ListCredentials(ctx context.Context, msg *capiv1_proto.ListCredentialsRequest) (*capiv1_proto.ListCredentialsResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
credentials.go too perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 !!!
Much nicer nicely done :)
@@ -0,0 +1,151 @@ | |||
package server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets rename this to common_test.go or something like that to make it clearer it just test stuff.
If its used by other packages we could go for test_common.go or something that can be exported (I think that works..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure 👍 I think it's used by the same package only :)
Closes #359