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

Implementing refs to clusterresources in postgres cluster #576

Closed
wants to merge 1 commit into from

Conversation

tengu-alt
Copy link
Collaborator

No description provided.

@tengu-alt tengu-alt self-assigned this Sep 25, 2023
@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch from ac29154 to 9d62caf Compare October 4, 2023 10:02
@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch 2 times, most recently from 7cb587f to 6294ada Compare October 18, 2023 11:53
@worryg0d worryg0d added the enhancement New feature or request label Oct 18, 2023
@tengu-alt tengu-alt changed the title [WIP] implementing refs to clusterresources in cluster Implementing refs to clusterresources in postgres cluster Oct 18, 2023
@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch 3 times, most recently from 7289f36 to 58ccceb Compare October 19, 2023 11:12
immutablePeeringFields{
DataCentreID: aws.DataCentreID,
},
immutablePeeringFields{},
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the DataCentreID is mutable or unused for now, it is not necessary to leave the immutablePeeringFields struct unfilled, just remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, fixed

ClusterID string `json:"clusterId"`
ClusterID string `json:"clusterId,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the customer doesn't have to fill in the clusterID, let's remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Other clusters are still using this field

Comment on lines -26 to +28
ClusterID string `json:"clusterId"`
ClusterID string `json:"clusterId,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the customer doesn't have to fill in the clusterID, let's remove it. Fix in all similar cases, please

Comment on lines 81 to 89
UserRefs []*UserReference `json:"userRefs,omitempty"`
ClusterBackups []*UserReference `json:"clusterBackups,omitempty"`
ClusterNetworkFirewallRules []*UserReference `json:"clusterNetworkFirewallRules,omitempty"`
AWSVPCPeerings []*UserReference `json:"awsVPCPeerings,omitempty"`
AWSSecurityGroupFirewallRules []*UserReference `json:"awsSecurityGroupFirewallRules,omitempty"`
ExclusionWindows []*UserReference `json:"exclusionWindows,omitempty"`
GCPVPCPeerings []*UserReference `json:"gcpVPCPeerings,omitempty"`
AzureVNetPeerings []*UserReference `json:"azureVNetPeerings,omitempty"`
//ClusterResourceRefs *ClusterResourceRefs `json:"clusterResourceRefs,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the ClusterResourceRefs struct here and rename the UserReference struct to something like NamespacedName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, fixed

@@ -195,7 +212,6 @@ func (pgs *PgSpec) ToInstAPI() *models.PGCluster {
PostgreSQLVersion: pgs.Version,
DataCentres: pgs.DCsToInstAPI(),
SynchronousModeStrict: pgs.SynchronousModeStrict,
Description: pgs.Description,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove the Description field from here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question

@@ -183,6 +267,7 @@ spec:
privateNetworkCluster:
type: boolean
resizeSettings:
description: ClusterResourceRefs *ClusterResourceRefs `json:"clusterResourceRefs,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks pretty strange to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was triggered by unnecessary comment

Comment on lines 77 to 79
if firewallRule.Annotations[models.ClusterIDAnnotation] == "" {
return models.ExitReconcile, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use something similar to clusterEvents as we did for cluster's users instead of entirely relying on annotations

Copy link
Collaborator Author

@tengu-alt tengu-alt Oct 23, 2023

Choose a reason for hiding this comment

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

agree, refactored

Comment on lines 20 to 23
"context"
"encoding/json"
"errors"
"github.com/instaclustr/operator/controllers/clusterresources"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sort imports please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines 85 to 86
reconcileResult := r.HandleDeleteFirewallRule(ctx, firewallRule, &l)
return reconcileResult, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

return

Suggested change
reconcileResult := r.HandleDeleteFirewallRule(ctx, firewallRule, &l)
return reconcileResult, nil
return r.HandleDeleteFirewallRule(ctx, firewallRule, &l), nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines 691 to 694
l.Info("PostgreSQL clusterresource was updated",
"Resource name:", ref.Name,
"Resource Kind:", kind,
"Event:", models.DeletingEvent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove those colons symbols here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch 2 times, most recently from 32779b0 to dcb12a6 Compare October 25, 2023 15:01
@testisnullus testisnullus self-requested a review October 25, 2023 19:02
Comment on lines +79 to +82
func (cb *ClusterBackup) DetachFromCluster() {

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete if unused, please

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was created to implement the custom interface Object which allows to work generic object status

Comment on lines 20 to 21
"github.com/instaclustr/operator/pkg/models"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
Copy link
Collaborator

Choose a reason for hiding this comment

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

imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -62,7 +62,7 @@ type CassandraSpec struct {
PasswordAndUserAuth bool `json:"passwordAndUserAuth,omitempty"`
Spark []*Spark `json:"spark,omitempty"`
BundledUseOnly bool `json:"bundledUseOnly,omitempty"`
UserRefs []*UserReference `json:"userRefs,omitempty"`
UserRefs []*NamespacedNameRef `json:"userRefs,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

NamespacedNameRef -> NamespacedName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed

@@ -195,7 +212,6 @@ func (pgs *PgSpec) ToInstAPI() *models.PGCluster {
PostgreSQLVersion: pgs.Version,
DataCentres: pgs.DCsToInstAPI(),
SynchronousModeStrict: pgs.SynchronousModeStrict,
Description: pgs.Description,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question

Comment on lines 85 to 87
if firewallRule.Annotations[models.ResourceStateAnnotation] == models.GenericEvent {
l.Info("AWS security group firewall rule event isn't handled",
"cluster ID", firewallRule.Spec.ClusterID,
"cluster ID", firewallRule.Status.ClusterID,
"type", firewallRule.Spec.Type,
"request", req,
"event", firewallRule.Annotations[models.ResourceStateAnnotation])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to handle the Generic Event if we rely on resource status?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree fixed

firewallRule.Status.ClusterEvent = models.DeletedEvent
err = r.Status().Patch(ctx, firewallRule, patch)
if err != nil {
l.Error(err, "Cannot patch AWS security group firewall rule status ", "ID", firewallRule.Status.ID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the space please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

"type", firewallRule.Spec.Type,
)

patch := firewallRule.NewPatch()

firewallRuleStatus, err := r.API.CreateFirewallRule(instaclustr.AWSSecurityGroupFirewallRuleEndpoint, &firewallRule.Spec)
firewallRuleStatus, err := r.API.CreateAWSSecurityGroupFirewallRule(instaclustr.AWSSecurityGroupFirewallRuleEndpoint, &firewallRule.Spec, firewallRule.Status.ClusterID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Form the endpoint inside API method please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

firewallRule.Status.ClusterEvent = models.DeletedEvent
err = r.Status().Patch(ctx, firewallRule, patch)
if err != nil {
l.Error(err, "Cannot patch cluster network firewall rule status ", "ID", firewallRule.Status.ID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove space, fix in all similar cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

resource = &clusterresourcesv1beta1.AzureVNetPeering{}
isCDC = true
default:
l.Info("Provided reference to resource that is not support deletion", "kind", kind, "resource", resource)
Copy link
Collaborator

Choose a reason for hiding this comment

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

deletion -> creation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 576 to 578
case "ClusterBackup":
resource = &clusterresourcesv1beta1.ClusterBackup{}
case "ClusterNetworkFirewallRule":
resource = &clusterresourcesv1beta1.ClusterNetworkFirewallRule{}
case "AWSVPCPeering":
resource = &clusterresourcesv1beta1.AWSVPCPeering{}
isCDC = true
case "AWSSecurityGroupFirewallRule":
resource = &clusterresourcesv1beta1.AWSSecurityGroupFirewallRule{}
case "ExclusionWindow":
resource = &clusterresourcesv1beta1.ExclusionWindow{}
case "GCPVPCPeering":
resource = &clusterresourcesv1beta1.GCPVPCPeering{}
isCDC = true
case "AzureVNetPeering":
resource = &clusterresourcesv1beta1.AzureVNetPeering{}
isCDC = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move to models?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, done

@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch 3 times, most recently from 4d628fb to b971a58 Compare October 27, 2023 13:24
err := r.Get(ctx, req, resource)
if err != nil {
if k8serrors.IsNotFound(err) {
l.Error(err, "Cannot create a cluster resource. The resource is not found", "request", req)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
l.Error(err, "Cannot create a cluster resource. The resource is not found", "request", req)
l.Error(err, "Provided resource is not found", "request", req)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

l.Error(err, "Cannot create a cluster resource. The resource is not found", "request", req)
return err
}
l.Error(err, "Cannot get cluster resource", "Resource", resource)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
l.Error(err, "Cannot get cluster resource", "Resource", resource)
l.Error(err, "Cannot get cluster resource", "request", req)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

}

l.Info("PostgreSQL clusterresource was patched",
"Resource name", ref.Name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should log and namespace too

Suggested change
"Resource name", ref.Name,
"reference", ref,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored

resource.DetachFromCluster()

err = r.Status().Patch(ctx, resource, patch)

Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this gap please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch from b971a58 to 513188d Compare October 27, 2023 13:35
@worryg0d worryg0d self-requested a review October 27, 2023 13:53
Comment on lines 3 to 6

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this message here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -37,6 +37,7 @@ type ClusterBackupStatus struct {
Progress string `json:"progress,omitempty"`
Start int `json:"start,omitempty"`
End int `json:"end,omitempty"`
ClusterID string `json:"clusterID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

In json it should be clusterId. Please change here and in all such cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines +87 to +95
type ClusterResourceRefs struct {
ClusterBackups []*NamespacedName `json:"clusterBackups,omitempty"`
ClusterNetworkFirewallRules []*NamespacedName `json:"clusterNetworkFirewallRules,omitempty"`
AWSVPCPeerings []*NamespacedName `json:"awsVPCPeerings,omitempty"`
AWSSecurityGroupFirewallRules []*NamespacedName `json:"awsSecurityGroupFirewallRules,omitempty"`
ExclusionWindows []*NamespacedName `json:"exclusionWindows,omitempty"`
GCPVPCPeerings []*NamespacedName `json:"gcpVPCPeerings,omitempty"`
AzureVNetPeerings []*NamespacedName `json:"azureVNetPeerings,omitempty"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all resources should be in the same namespace as a cluster. So in general the entity name would be sufficient. But this solution is also ok

Comment on lines +87 to +95
type ClusterResourceRefs struct {
ClusterBackups []*NamespacedName `json:"clusterBackups,omitempty"`
ClusterNetworkFirewallRules []*NamespacedName `json:"clusterNetworkFirewallRules,omitempty"`
AWSVPCPeerings []*NamespacedName `json:"awsVPCPeerings,omitempty"`
AWSSecurityGroupFirewallRules []*NamespacedName `json:"awsSecurityGroupFirewallRules,omitempty"`
ExclusionWindows []*NamespacedName `json:"exclusionWindows,omitempty"`
GCPVPCPeerings []*NamespacedName `json:"gcpVPCPeerings,omitempty"`
AzureVNetPeerings []*NamespacedName `json:"azureVNetPeerings,omitempty"`
}
Copy link
Contributor

@DoodgeMatvey DoodgeMatvey Oct 31, 2023

Choose a reason for hiding this comment

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

Also, I think a better solution would be to store resource references as map[string][]name or map[string][]namespacedName where string will be the resource name. Because when there are a lot of resources, I think such a solution will not be beautiful and a high-quality architectural solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ResorceRefs must be an object for maintenance improvement

Copy link
Contributor

@DoodgeMatvey DoodgeMatvey Nov 3, 2023

Choose a reason for hiding this comment

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

I just think this is not an improvement but a deterioration because there are a lot of duplicate unnecessary code not only in this place, also adding new resources will be an issue. This approach can be discussed with @taaraora and other team members @testisnullus @OleksiienkoMykyta @worryg0d @RostislavPorohnya @ribaraka

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we can simplify the code and make the resourceRefs architecture cleaner than it is using the suggested approach by Matvii, I think it will be a way better solution for now

Copy link
Collaborator Author

@tengu-alt tengu-alt Nov 3, 2023

Choose a reason for hiding this comment

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

I just think this is not an improvement but a deterioration because there are a lot of duplicate unnecessary code not only in this place, also adding new resources will be an issue. This approach can be discussed with @taaraora and other team members @testisnullus @OleksiienkoMykyta @worryg0d @RostislavPorohnya @ribaraka

I have already discussed it with @taaraora and we decided to store refs as an object

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@@ -1,7 +1,7 @@
apiVersion: clusterresources.instaclustr.com/v1beta1
kind: ClusterBackup
metadata:
name: clusterbackup-sample
name: clusterbackup-sample-two
Copy link
Contributor

Choose a reason for hiding this comment

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

I think default clusterbackup-sample is better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree fixed

Comment on lines 82 to 81
switch aws.Annotations[models.ResourceStateAnnotation] {
case models.UpdatingEvent:
return r.handleUpdatePeering(ctx, aws, l), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

why didn't you make the same logic with ResourceState for update event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, refactored

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, refactored

Comment on lines +77 to 83
switch ew.Status.ResourceState {
case models.CreatingEvent:
return r.handleCreateWindow(ctx, ew, l), nil
case models.DeletingEvent:
return r.handleDeleteWindow(ctx, ew, l), nil
default:
l.Info("event isn't handled",
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use switch instead of if for all such cases so that the code will be persistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

ew.Status.ResourceState = models.DeletedEvent
err = r.Status().Patch(ctx, ew, patch)
if err != nil {
l.Error(err, "cannot patch Exclusion Window resource status",
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot patch Exclusion Window -> Cannot patch Exclusion Window.
Start such error messages from UpperCase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +500 to +506
r.HandleResourceEvent(newObj, models.ClusterbackupRef, oldObjSpec.ClusterResources.ClusterBackups, newObj.Spec.ClusterResources.ClusterBackups)
r.HandleResourceEvent(newObj, models.ClusterNetworkFirewallRuleref, oldObjSpec.ClusterResources.ClusterNetworkFirewallRules, newObj.Spec.ClusterResources.ClusterNetworkFirewallRules)
r.HandleResourceEvent(newObj, models.AWSVPCPeeringRef, oldObjSpec.ClusterResources.AWSVPCPeerings, newObj.Spec.ClusterResources.AWSVPCPeerings)
r.HandleResourceEvent(newObj, models.AWSSecurityGroupFirewallRuleRef, oldObjSpec.ClusterResources.AWSSecurityGroupFirewallRules, newObj.Spec.ClusterResources.AWSSecurityGroupFirewallRules)
r.HandleResourceEvent(newObj, models.ExclusionWindowRef, oldObjSpec.ClusterResources.ExclusionWindows, newObj.Spec.ClusterResources.ExclusionWindows)
r.HandleResourceEvent(newObj, models.GCPVPCPeeringRef, oldObjSpec.ClusterResources.GCPVPCPeerings, newObj.Spec.ClusterResources.GCPVPCPeerings)
r.HandleResourceEvent(newObj, models.AzureVNetPeeringRef, oldObjSpec.ClusterResources.AzureVNetPeerings, newObj.Spec.ClusterResources.AzureVNetPeerings)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of using the map that I described above, there would be no need to write such code

Comment on lines 518 to 521
var exist bool
for _, oldRef := range oldRefs {
if *ref == *oldRef {
exist = true
break
}
}

if exist {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move it to separate func, because there are duplication below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is no duplication, we check arrays on specific event(creation/deletion) and iterating by different ways

Copy link
Contributor

Choose a reason for hiding this comment

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

oldRefs and newRefs have the same type and you did the same in these 2 peaces of code. Please move code that I commented on lines +518 to +528 to the seperate function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, fixed

@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch from 513188d to 815ef9e Compare November 3, 2023 13:36
@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch from 815ef9e to d999585 Compare November 3, 2023 13:41
Comment on lines 518 to 521
var exist bool
for _, oldRef := range oldRefs {
if *ref == *oldRef {
exist = true
break
}
}

if exist {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

oldRefs and newRefs have the same type and you did the same in these 2 peaces of code. Please move code that I commented on lines +518 to +528 to the seperate function

@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch 2 times, most recently from 452ca79 to 3e91cac Compare November 6, 2023 11:14
@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch from 3e91cac to 40309f0 Compare November 6, 2023 11:29
@@ -36,7 +38,9 @@ type ExclusionWindowSpec struct {

// ExclusionWindowStatus defines the observed state of ExclusionWindow
type ExclusionWindowStatus struct {
ID string `json:"id"`
ID string `json:"id,omitempty"`
ClusterID string `json:"clusterID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename clusterID -> clusterId here and in all such cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

azure.Status.ResourceState = models.DeletedEvent
err = r.Status().Patch(ctx, azure, patch)
if err != nil {
l.Error(err, "cannot patch Azure VNet Peering resource status",
Copy link
Contributor

Choose a reason for hiding this comment

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

Start such error messages from capitalized letter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

l := log.FromContext(ctx)

for _, ref := range newRefs {
exist := isClusterResourceRefExist(ref, oldRefs)
Copy link
Contributor

Choose a reason for hiding this comment

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

isClusterResourceRefExist -> isClusterResourceRefExists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@tengu-alt tengu-alt force-pushed the implement-clusterresource-refs-clusters branch from 40309f0 to 3a74ba1 Compare November 6, 2023 12:15
@@ -106,6 +106,14 @@ const (
CassandraAppType = "APACHE_CASSANDRA"
SparkAppType = "SPARK"

ClusterbackupRef = "ClusterBackup"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ClusterBackupRef

Comment on lines +49 to +52
clusterBackups:
# - namespace: default
# name: clusterbackup-sample
clusterNetworkFirewallRules:
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove 'cluster' prefixes from here

@ribaraka ribaraka closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants