Cadence packaged provisioning was refactored #726
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
testisnullus
force-pushed
the
cadence-packaged-solution-refactor
branch
4 times, most recently
from
e9c66ee
to
2504b1b
Compare
worryg0d
reviewed
Feb 28, 2024
Comment on lines
353
to
355
| if pp.UseAdvancedVisibility != old[i].UseAdvancedVisibility || | ||
| pp.SolutionSize != old[i].SolutionSize { | ||
| return models.ErrImmutablePackagedProvisioning | ||
| } |
There was a problem hiding this comment.
You may compare it this way because PackagedProvisioning is comparable type
Suggested change
| if pp.UseAdvancedVisibility != old[i].UseAdvancedVisibility || | |
| pp.SolutionSize != old[i].SolutionSize { | |
| return models.ErrImmutablePackagedProvisioning | |
| } | |
| if *pp != *old[i] { | |
| return models.ErrImmutablePackagedProvisioning | |
| } |
Comment on lines
496
to
499
| contains := validation.Contains(pp.SolutionSize, models.SolutionSizes) | ||
| if !contains { | ||
| return fmt.Errorf("the %s solution size is not supported. Supported ones: %v", pp.SolutionSize, models.SolutionSizes) | ||
| } |
There was a problem hiding this comment.
You may use kubebuilder enum annotations for this type of validation. WDYT?
worryg0d
reviewed
Feb 28, 2024
| return "", nil | ||
| } | ||
|
|
||
| func calculateAvailableNetworks(cadenceNetwork string) ([]string, error) { |
There was a problem hiding this comment.
You cannot specify network when you use packaged provisioning for cadence.
testisnullus
force-pushed
the
cadence-packaged-solution-refactor
branch
from
2504b1b
to
c49e3c3
Compare
testisnullus
force-pushed
the
cadence-packaged-solution-refactor
branch
2 times, most recently
from
14e0905
to
e840aa5
Compare
worryg0d
reviewed
Feb 29, 2024
controllers/clusters/helpers.go
Outdated
| @@ -238,3 +241,36 @@ func reconcileExternalChanges(c client.Client, r record.EventRecorder, obj Objec | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func CalculateAvailableNetworks(cadenceNetwork string) ([]string, error) { | |||
There was a problem hiding this comment.
I believe this one should not be exported
worryg0d
approved these changes
Feb 29, 2024
ribaraka
requested changes
Feb 29, 2024
Comment on lines
800
to
806
| func (r *CadenceReconciler) reconcileNetwork(availableCIDRs []string, app string) (string, error) { | ||
| switch app { | ||
| case models.CassandraAppKind: | ||
| return availableCIDRs[1], nil | ||
| case models.KafkaAppKind: | ||
| return availableCIDRs[2], nil | ||
| case models.OpenSearchAppKind: | ||
| return availableCIDRs[3], nil | ||
| } | ||
|
|
||
| return &v1beta1.Cassandra{ | ||
| TypeMeta: typeMeta, | ||
| ObjectMeta: metadata, | ||
| Spec: spec, | ||
| }, nil | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
It seems that errors are not being returned in this function. Perhaps consider removing the error from the return parameters
testisnullus
force-pushed
the
cadence-packaged-solution-refactor
branch
from
e840aa5
to
e5ca7ac
Compare
tengu-alt
approved these changes
Mar 1, 2024
ribaraka
requested changes
Mar 1, 2024
Comment on lines
668
to
726
| func (r *CadenceReconciler) reconcileAppDCs(c *v1beta1.Cadence, availableCIDRs []string, app string) (any, error) { | ||
| network := r.reconcileNetwork(availableCIDRs, app) | ||
|
|
||
| genericDataCentreSpec := v1beta1.GenericDataCentreSpec{ | ||
| Region: c.Spec.DataCentres[0].Region, | ||
| CloudProvider: c.Spec.DataCentres[0].CloudProvider, | ||
| ProviderAccountName: c.Spec.DataCentres[0].ProviderAccountName, | ||
| Network: network, | ||
| } | ||
|
|
||
| switch app { | ||
| case models.CassandraAppKind: | ||
| var privateIPBroadcast bool | ||
| if c.Spec.PrivateNetwork { | ||
| privateIPBroadcast = true | ||
| } | ||
|
|
||
| genericDataCentreSpec.Name = models.CassandraChildDCName | ||
| cassandraDCs := []*v1beta1.CassandraDataCentre{ | ||
| { | ||
| Email: c.Spec.TwoFactorDelete[0].Email, | ||
| Phone: c.Spec.TwoFactorDelete[0].Phone, | ||
| GenericDataCentreSpec: genericDataCentreSpec, | ||
| NodeSize: c.Spec.CalculateNodeSize( | ||
| c.Spec.DataCentres[0].CloudProvider, | ||
| c.Spec.PackagedProvisioning[0].SolutionSize, | ||
| models.CassandraAppKind, | ||
| ), | ||
| NodesNumber: 3, | ||
| ReplicationFactor: 3, | ||
| PrivateIPBroadcastForDiscovery: privateIPBroadcast, | ||
| }, | ||
| } | ||
|
|
||
| return cassandraDCs, nil | ||
|
|
||
| case models.KafkaAppKind: | ||
| genericDataCentreSpec.Name = models.KafkaChildDCName | ||
| kafkaDCs := []*v1beta1.KafkaDataCentre{ | ||
| { | ||
| GenericDataCentreSpec: genericDataCentreSpec, | ||
| NodeSize: c.Spec.CalculateNodeSize( | ||
| c.Spec.DataCentres[0].CloudProvider, | ||
| c.Spec.PackagedProvisioning[0].SolutionSize, | ||
| models.KafkaAppKind, | ||
| ), | ||
| NodesNumber: 3, | ||
| }, | ||
| } | ||
| return kafkaDCs, nil | ||
|
|
||
| case models.OpenSearchAppKind: | ||
| genericDataCentreSpec.Name = models.OpenSearchChildDCName | ||
| openSearchDCs := []*v1beta1.OpenSearchDataCentre{ | ||
| { | ||
| GenericDataCentreSpec: genericDataCentreSpec, | ||
| NumberOfRacks: 3, | ||
| }, | ||
| } | ||
| return openSearchDCs, nil | ||
| } | ||
| var cassNodeSize, network string | ||
| var cassNodesNumber, cassReplicationFactor int | ||
| var cassPrivateIPBroadcastForDiscovery, cassPasswordAndUserAuth bool | ||
| for _, pp := range c.Spec.PackagedProvisioning { | ||
| cassNodeSize = pp.BundledCassandraSpec.NodeSize | ||
| network = pp.BundledCassandraSpec.Network | ||
| cassNodesNumber = pp.BundledCassandraSpec.NodesNumber | ||
| cassReplicationFactor = pp.BundledCassandraSpec.ReplicationFactor | ||
| cassPrivateIPBroadcastForDiscovery = pp.BundledCassandraSpec.PrivateIPBroadcastForDiscovery | ||
| cassPasswordAndUserAuth = pp.BundledCassandraSpec.PasswordAndUserAuth | ||
|
|
||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
This function doesn't seem to return any errors. Please remove error parameter
testisnullus
force-pushed
the
cadence-packaged-solution-refactor
branch
2 times, most recently
from
c0ffaa6
to
7ee8400
Compare
testisnullus
force-pushed
the
cadence-packaged-solution-refactor
branch
from
7ee8400
to
65e399a
Compare
ribaraka
approved these changes
Mar 4, 2024
This was referenced Mar 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.