Skip to content

Commit

Permalink
fix code smell (#28)
Browse files Browse the repository at this point in the history
  • Loading branch information
songleo authored May 14, 2020
1 parent 48adc85 commit 4f47e11
Show file tree
Hide file tree
Showing 13 changed files with 261 additions and 132 deletions.
27 changes: 24 additions & 3 deletions cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,12 @@ func main() {
}

// Setup Scheme for observatorium resources
schemeBuilder := &scheme.Builder{GroupVersion: schema.GroupVersion{Group: "core.observatorium.io", Version: "v1alpha1"}}
schemeBuilder := &scheme.Builder{
GroupVersion: schema.GroupVersion{
Group: "core.observatorium.io",
Version: "v1alpha1",
},
}
schemeBuilder.Register(&observatoriumAPIs.Observatorium{}, &observatoriumAPIs.ObservatoriumList{})
if err := schemeBuilder.AddToScheme(mgr.GetScheme()); err != nil {
log.Error(err, "")
Expand Down Expand Up @@ -156,8 +161,24 @@ func addMetrics(ctx context.Context, cfg *rest.Config, namespace string) {

// Add to the below struct any other metrics ports you want to expose.
servicePorts := []v1.ServicePort{
{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
{Port: operatorMetricsPort, Name: metrics.CRPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: operatorMetricsPort}},
{
Port: metricsPort,
Name: metrics.OperatorPortName,
Protocol: v1.ProtocolTCP,
TargetPort: intstr.IntOrString{
Type: intstr.Int,
IntVal: metricsPort,
},
},
{
Port: operatorMetricsPort,
Name: metrics.CRPortName,
Protocol: v1.ProtocolTCP,
TargetPort: intstr.IntOrString{
Type: intstr.Int,
IntVal: operatorMetricsPort,
},
},
}

// Create Service object to expose the metrics port(s).
Expand Down
16 changes: 8 additions & 8 deletions pkg/controller/multiclustermonitoring/default_conf.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,35 @@ import (
)

const (
DEFAULT_VERSION = "latest"
DEFAULT_IMG_REPO = "quay.io/open-cluster-management"
DEFAULT_IMG_PULL_SECRET = "quay-secret"
DEFAULT_STORAGE_CLASS = "gp2"
defaultVersion = "latest"
defaultImgRepo = "quay.io/open-cluster-management"
defaultImgPullSecret = "quay-secret"
defaultStorageClass = "gp2"
)

func addDefaultConfig(c client.Client, mcm *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {
if mcm.Spec.Version == "" {
mcm.Spec.Version = DEFAULT_VERSION
mcm.Spec.Version = defaultVersion
}

if mcm.Spec.ImageRepository == "" {
mcm.Spec.ImageRepository = DEFAULT_IMG_REPO
mcm.Spec.ImageRepository = defaultImgRepo
}

if string(mcm.Spec.ImagePullPolicy) == "" {
mcm.Spec.ImagePullPolicy = corev1.PullAlways
}

if mcm.Spec.ImagePullSecret == "" {
mcm.Spec.ImagePullSecret = DEFAULT_IMG_PULL_SECRET
mcm.Spec.ImagePullSecret = defaultImgPullSecret
}

if mcm.Spec.NodeSelector == nil {
mcm.Spec.NodeSelector = &monitoringv1alpha1.NodeSelector{}
}

if mcm.Spec.StorageClass == "" {
mcm.Spec.StorageClass = DEFAULT_STORAGE_CLASS
mcm.Spec.StorageClass = defaultStorageClass
}

if mcm.Spec.Observatorium == nil {
Expand Down
16 changes: 8 additions & 8 deletions pkg/controller/multiclustermonitoring/default_conf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,28 @@ func TestAddDefaultConfig(t *testing.T) {
t.Errorf("Should return nil for result (%v) and err (%v)", result, err)
}

if mcm.Spec.Version != DEFAULT_VERSION {
t.Errorf("Version (%v) is not the expected (%v)", mcm.Spec.Version, DEFAULT_VERSION)
if mcm.Spec.Version != defaultVersion {
t.Errorf("Version (%v) is not the expected (%v)", mcm.Spec.Version, defaultVersion)
}

if mcm.Spec.ImageRepository != DEFAULT_IMG_REPO {
t.Errorf("ImageRepository (%v) is not the expected (%v)", mcm.Spec.ImageRepository, DEFAULT_IMG_REPO)
if mcm.Spec.ImageRepository != defaultImgRepo {
t.Errorf("ImageRepository (%v) is not the expected (%v)", mcm.Spec.ImageRepository, defaultImgRepo)
}

if string(mcm.Spec.ImagePullPolicy) != string(corev1.PullAlways) {
t.Errorf("ImagePullPolicy (%v) is not the expected (%v)", mcm.Spec.ImagePullPolicy, corev1.PullAlways)
}

if mcm.Spec.ImagePullSecret != DEFAULT_IMG_PULL_SECRET {
t.Errorf("ImagePullSecret (%v) is not the expected (%v)", mcm.Spec.ImagePullSecret, DEFAULT_IMG_PULL_SECRET)
if mcm.Spec.ImagePullSecret != defaultImgPullSecret {
t.Errorf("ImagePullSecret (%v) is not the expected (%v)", mcm.Spec.ImagePullSecret, defaultImgPullSecret)
}

if mcm.Spec.NodeSelector == nil {
t.Errorf("NodeSelector (%v) is not the expected (non-nil)", mcm.Spec.NodeSelector)
}

if mcm.Spec.StorageClass != DEFAULT_STORAGE_CLASS {
t.Errorf("StorageClass (%v) is not the expected (%v)", mcm.Spec.StorageClass, DEFAULT_STORAGE_CLASS)
if mcm.Spec.StorageClass != defaultStorageClass {
t.Errorf("StorageClass (%v) is not the expected (%v)", mcm.Spec.StorageClass, defaultStorageClass)
}

if mcm.Spec.Observatorium == nil {
Expand Down
29 changes: 22 additions & 7 deletions pkg/controller/multiclustermonitoring/grafana.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ type GrafanaDatasource struct {

// GenerateGrafanaDataSource is used to generate the GrafanaDatasource as a secret.
// the GrafanaDatasource points to observatorium api gateway service
func GenerateGrafanaDataSource(client client.Client, scheme *runtime.Scheme, monitoring *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {
func GenerateGrafanaDataSource(
client client.Client,
scheme *runtime.Scheme,
monitoring *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {

grafanaDatasources, err := json.MarshalIndent(GrafanaDatasources{
APIVersion: 1,
Expand All @@ -47,15 +50,15 @@ func GenerateGrafanaDataSource(client client.Client, scheme *runtime.Scheme, mon
Name: "Observatorium",
Type: "prometheus",
Access: "proxy",
URL: "http://" + monitoring.Name + observatoriumPartoOfName + "-observatorium-api:8080/api/metrics/v1",
URL: "http://" + monitoring.Name + obsPartoOfName + "-observatorium-api:8080/api/metrics/v1",
},
},
}, "", " ")
if err != nil {
return &reconcile.Result{}, err
}

grafanaDataSourceSecret := &corev1.Secret{
dsSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "grafana-datasources",
Namespace: monitoring.Namespace,
Expand All @@ -67,16 +70,28 @@ func GenerateGrafanaDataSource(client client.Client, scheme *runtime.Scheme, mon
}

// Set MultiClusterMonitoring instance as the owner and controller
if err = controllerutil.SetControllerReference(monitoring, grafanaDataSourceSecret, scheme); err != nil {
if err = controllerutil.SetControllerReference(monitoring, dsSecret, scheme); err != nil {
return &reconcile.Result{}, err
}

// Check if this already exists
grafanaDSFound := &corev1.Secret{}
err = client.Get(context.TODO(), types.NamespacedName{Name: grafanaDataSourceSecret.Name, Namespace: grafanaDataSourceSecret.Namespace}, grafanaDSFound)
err = client.Get(
context.TODO(),
types.NamespacedName{
Name: dsSecret.Name,
Namespace: dsSecret.Namespace,
},
grafanaDSFound,
)

if err != nil && errors.IsNotFound(err) {
log.Info("Creating a new grafana datasource secret", "grafanaDataSourceSecret.Namespace", grafanaDataSourceSecret.Namespace, "grafanaDataSourceSecret.Name", grafanaDataSourceSecret.Name)
err = client.Create(context.TODO(), grafanaDataSourceSecret)
log.Info("Creating a new grafana datasource secret",
"dsSecret.Namespace", dsSecret.Namespace,
"dsSecret.Name", dsSecret.Name,
)

err = client.Create(context.TODO(), dsSecret)
if err != nil {
return &reconcile.Result{}, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,19 @@ var log = logf.Log.WithName("controller_multiclustermonitoring")
* business logic. Delete these comments after modifying this file.*
*/

// Add creates a new MultiClusterMonitoring Controller and adds it to the Manager. The Manager will set fields on the Controller
// and Start it when the Manager is Started.
// Add creates a new MultiClusterMonitoring Controller and adds it to the Manager. The Manager will set fields on
// the Controller and Start it when the Manager is Started.
func Add(mgr manager.Manager) error {
return add(mgr, newReconciler(mgr))
}

// newReconciler returns a new reconcile.Reconciler
func newReconciler(mgr manager.Manager) reconcile.Reconciler {
return &ReconcileMultiClusterMonitoring{client: mgr.GetClient(), apiReader: mgr.GetAPIReader(), scheme: mgr.GetScheme()}
return &ReconcileMultiClusterMonitoring{
client: mgr.GetClient(),
apiReader: mgr.GetAPIReader(),
scheme: mgr.GetScheme(),
}
}

// add adds a new Controller to mgr with r as the reconcile.Reconciler
Expand Down Expand Up @@ -109,9 +113,9 @@ type ReconcileMultiClusterMonitoring struct {
scheme *runtime.Scheme
}

// Reconcile reads that state of the cluster for a MultiClusterMonitoring object and makes changes based on the state read
// and what is in the MultiClusterMonitoring.Spec
// TODO(user): Modify this Reconcile function to implement your Controller logic. This example creates
// Reconcile reads that state of the cluster for a MultiClusterMonitoring object and makes changes
// based on the state read and what is in the MultiClusterMonitoring.Spec
// Modify this Reconcile function to implement your Controller logic. This example creates
// a Pod as an example
// Note:
// The Controller will requeue the Request to be processed again if the returned error is non-nil or
Expand Down Expand Up @@ -193,7 +197,9 @@ func (r *ReconcileMultiClusterMonitoring) Reconcile(request reconcile.Request) (
return reconcile.Result{Requeue: true}, nil
}

func (r *ReconcileMultiClusterMonitoring) UpdateStatus(mcm *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {
func (r *ReconcileMultiClusterMonitoring) UpdateStatus(
mcm *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {

reqLogger := log.WithValues("Request.Namespace", mcm.Namespace, "Request.Name", mcm.Name)

deployList := &appsv1.DeploymentList{}
Expand All @@ -203,7 +209,10 @@ func (r *ReconcileMultiClusterMonitoring) UpdateStatus(mcm *monitoringv1alpha1.M
}
err := r.client.List(context.TODO(), deployList, listOpts...)
if err != nil {
reqLogger.Error(err, "Failed to list deployments.", "MultiClusterMonitoring.Namespace", mcm.Namespace, "MemcaMultiClusterMonitoringched.Name", mcm.Name)
reqLogger.Error(err, "Failed to list deployments.",
"MultiClusterMonitoring.Namespace", mcm.Namespace,
"MemcaMultiClusterMonitoringched.Name", mcm.Name,
)
return &reconcile.Result{}, err
}

Expand Down
33 changes: 18 additions & 15 deletions pkg/controller/multiclustermonitoring/object_storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,32 @@ import (
)

const (
DEFAULT_OBJ_STORAGE_TYPE = "minio"
DEFAULT_OBJ_STORAGE_BUCKET = "thanos"
DEFAULT_OBJ_STORAGE_ENDPOINT = "minio:9000"
DEFAULT_OBJ_STORAGE_INSECURE = true
DEFAULT_OBJ_STORAGE_ACCESSKEY = "minio"
DEFAULT_OBJ_STORAGE_SECRETKEY = "minio123"
DEFAULT_OBJ_STORAGE_STORAGE = "1Gi"
defaultObjStorageType = "minio"
defaultObjStorageBucket = "thanos"
defaultObjStorageEndpoint = "minio:9000"
defaultObjStorageInsecure = true
defaultObjStorageAccesskey = "minio"
defaultObjStorageSecretkey = "minio123"
defaultObjStorageStorage = "1Gi"
)

func newDefaultObjectStorageConfigSpec() *monitoringv1alpha1.ObjectStorageConfigSpec {
spec := &monitoringv1alpha1.ObjectStorageConfigSpec{}
spec.Type = DEFAULT_OBJ_STORAGE_TYPE
spec.Config.Bucket = DEFAULT_OBJ_STORAGE_BUCKET
spec.Config.Endpoint = DEFAULT_OBJ_STORAGE_ENDPOINT
spec.Config.Insecure = DEFAULT_OBJ_STORAGE_INSECURE
spec.Config.AccessKey = DEFAULT_OBJ_STORAGE_ACCESSKEY
spec.Config.SecretKey = DEFAULT_OBJ_STORAGE_SECRETKEY
spec.Config.Storage = DEFAULT_OBJ_STORAGE_STORAGE
spec.Type = defaultObjStorageType
spec.Config.Bucket = defaultObjStorageBucket
spec.Config.Endpoint = defaultObjStorageEndpoint
spec.Config.Insecure = defaultObjStorageInsecure
spec.Config.AccessKey = defaultObjStorageAccesskey
spec.Config.SecretKey = defaultObjStorageSecretkey
spec.Config.Storage = defaultObjStorageStorage

return spec
}

func updateObjStorageConfig(c client.Client, mcm *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {
func updateObjStorageConfig(
c client.Client,
mcm *monitoringv1alpha1.MultiClusterMonitoring) (*reconcile.Result, error) {

// Check valid object storage type
if mcm.Spec.ObjectStorageConfigSpec != nil {
objStorageType := mcm.Spec.ObjectStorageConfigSpec.Type
Expand Down
40 changes: 20 additions & 20 deletions pkg/controller/multiclustermonitoring/object_storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,32 +24,32 @@ func NewFakeClient(mcm *monitoringv1alpha1.MultiClusterMonitoring) client.Client
func TestNewDefaultObjectStorageConfigSpec(t *testing.T) {
spec := newDefaultObjectStorageConfigSpec()

if spec.Type != DEFAULT_OBJ_STORAGE_TYPE {
t.Errorf("Type (%v) is not the expected (%v)", spec.Type, DEFAULT_OBJ_STORAGE_TYPE)
if spec.Type != defaultObjStorageType {
t.Errorf("Type (%v) is not the expected (%v)", spec.Type, defaultObjStorageType)
}

if spec.Config.Bucket != DEFAULT_OBJ_STORAGE_BUCKET {
t.Errorf("Bucket (%v) is not the expected (%v)", spec.Config.Bucket, DEFAULT_OBJ_STORAGE_BUCKET)
if spec.Config.Bucket != defaultObjStorageBucket {
t.Errorf("Bucket (%v) is not the expected (%v)", spec.Config.Bucket, defaultObjStorageBucket)
}

if spec.Config.Endpoint != DEFAULT_OBJ_STORAGE_ENDPOINT {
t.Errorf("Endpoint (%v) is not the expected (%v)", spec.Config.Endpoint, DEFAULT_OBJ_STORAGE_ENDPOINT)
if spec.Config.Endpoint != defaultObjStorageEndpoint {
t.Errorf("Endpoint (%v) is not the expected (%v)", spec.Config.Endpoint, defaultObjStorageEndpoint)
}

if spec.Config.Insecure != DEFAULT_OBJ_STORAGE_INSECURE {
t.Errorf("Insecure (%v) is not the expected (%v)", spec.Config.Insecure, DEFAULT_OBJ_STORAGE_INSECURE)
if spec.Config.Insecure != defaultObjStorageInsecure {
t.Errorf("Insecure (%v) is not the expected (%v)", spec.Config.Insecure, defaultObjStorageInsecure)
}

if spec.Config.AccessKey != DEFAULT_OBJ_STORAGE_ACCESSKEY {
t.Errorf("AccessKey (%v) is not the expected (%v)", spec.Config.AccessKey, DEFAULT_OBJ_STORAGE_ACCESSKEY)
if spec.Config.AccessKey != defaultObjStorageAccesskey {
t.Errorf("AccessKey (%v) is not the expected (%v)", spec.Config.AccessKey, defaultObjStorageAccesskey)
}

if spec.Config.SecretKey != DEFAULT_OBJ_STORAGE_SECRETKEY {
t.Errorf("SecretKey (%v) is not the expected (%v)", spec.Config.SecretKey, DEFAULT_OBJ_STORAGE_SECRETKEY)
if spec.Config.SecretKey != defaultObjStorageSecretkey {
t.Errorf("SecretKey (%v) is not the expected (%v)", spec.Config.SecretKey, defaultObjStorageSecretkey)
}

if spec.Config.Storage != DEFAULT_OBJ_STORAGE_STORAGE {
t.Errorf("Storage (%v) is not the expected (%v)", spec.Config.Storage, DEFAULT_OBJ_STORAGE_STORAGE)
if spec.Config.Storage != defaultObjStorageStorage {
t.Errorf("Storage (%v) is not the expected (%v)", spec.Config.Storage, defaultObjStorageStorage)
}

}
Expand All @@ -74,10 +74,10 @@ func TestCheckObjStorageConfig(t *testing.T) {
t.Errorf("Failed to check valid object storage type: result: (%v) err: (%v)", result, err)
}

mcm.Spec.ObjectStorageConfigSpec.Type = DEFAULT_OBJ_STORAGE_TYPE
mcm.Spec.ObjectStorageConfigSpec.Type = defaultObjStorageType
result, err = updateObjStorageConfig(NewFakeClient(mcm), mcm)
if result != nil || err != nil {
t.Errorf("(%v) should be a valid type", DEFAULT_OBJ_STORAGE_TYPE)
t.Errorf("(%v) should be a valid type", defaultObjStorageType)
}

mcm.Spec.ObjectStorageConfigSpec.Type = "s3"
Expand All @@ -87,12 +87,12 @@ func TestCheckObjStorageConfig(t *testing.T) {
}

updateObjStorageConfig(NewFakeClient(mcm), mcm)
if mcm.Spec.ObjectStorageConfigSpec.Config.Bucket != DEFAULT_OBJ_STORAGE_BUCKET {
t.Errorf("Bucket (%v) is not the expected (%v)", mcm.Spec.ObjectStorageConfigSpec.Config.Bucket, DEFAULT_OBJ_STORAGE_BUCKET)
if mcm.Spec.ObjectStorageConfigSpec.Config.Bucket != defaultObjStorageBucket {
t.Errorf("Bucket (%v) is not the expected (%v)", mcm.Spec.ObjectStorageConfigSpec.Config.Bucket, defaultObjStorageBucket)
}

mcm.Spec.ObjectStorageConfigSpec = newDefaultObjectStorageConfigSpec()
if mcm.Spec.ObjectStorageConfigSpec.Config.Endpoint != DEFAULT_OBJ_STORAGE_ENDPOINT {
t.Errorf("Endpoint (%v) is not the expected (%v)", mcm.Spec.ObjectStorageConfigSpec.Config.Endpoint, DEFAULT_OBJ_STORAGE_ENDPOINT)
if mcm.Spec.ObjectStorageConfigSpec.Config.Endpoint != defaultObjStorageEndpoint {
t.Errorf("Endpoint (%v) is not the expected (%v)", mcm.Spec.ObjectStorageConfigSpec.Config.Endpoint, defaultObjStorageEndpoint)
}
}
Loading

0 comments on commit 4f47e11

Please sign in to comment.