Added CivoNetwork Support #57
Conversation
apis/civo/network/v1alpha1/types.go
Outdated
| NameServersV4 []string `json:"nameservers_v4"` | ||
|
|
||
| // Details regarding current state of the bucket. | ||
| Conditions []metav1.Condition `json:"conditions"` |
There was a problem hiding this comment.
Do we need this again or can we re-use the ones Crossplane gives us?
apis/civo/network/v1alpha1/types.go
Outdated
| ID string `json:"id"` | ||
| Name string `json:"name"` | ||
| Default bool `json:"default"` | ||
| CIDR string `json:"cidr"` |
There was a problem hiding this comment.
Lot of these fields belong in the spec, not Observation like CIDR, Nameservers, Label
There was a problem hiding this comment.
The Go SDK accepts only labels as a param while creating a network, CIDR, Nameservers and all cannot be passed
|
|
| // CivoNetworkObservation are the observable fields of a CivoNetwork. | ||
| type CivoNetworkObservation struct { | ||
| ID string `json:"id"` | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
Name is repeated ?
There was a problem hiding this comment.
Nope, Name and label are two diff things
{
"id": "2d2febbd-8a93-45ea-923f-936c8449611c",
"name": "cust-default-d3daf04d-7505ea9087ce",
"default": true,
"cidr": "192.168.1.0/24",
"label": "default",
"status": "Active",
"ipv4_enabled": true,
"nameservers_v4": [
"8.8.8.8",
"1.1.1.1"
]
},
| ID string `json:"id"` | ||
| Name string `json:"name"` | ||
| Default bool `json:"default"` | ||
| Label string `json:"label"` |
There was a problem hiding this comment.
Description on fields is missing. What is the difference between Label and Name?
| @@ -20,10 +20,12 @@ import ( | |||
| "os" | |||
| "path/filepath" | |||
|
|
|||
| "github.com/crossplane-contrib/provider-civo/internal/controller/civonetwork" | |||
There was a problem hiding this comment.
Import order should be std, 3rd party, local pacakges.
go.mod
Outdated
| @@ -3,7 +3,7 @@ module github.com/crossplane-contrib/provider-civo | |||
| go 1.22 | |||
|
|
|||
| require ( | |||
| github.com/civo/civogo v0.2.68 | |||
| github.com/civo/civogo v0.3.28 | |||
There was a problem hiding this comment.
.65 seems to be latest
There was a problem hiding this comment.
.65 have that KubernetesPoolConfig issue. I'll fix that in a sepearate PR
| return managed.ExternalObservation{}, errors.New(errNotCivoNetwork) | ||
| } | ||
| civoNetwork, err := e.civoClient.GetNetwork(cr.Spec.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
There could be a networking issue on client side that leads to failure of getting network details. We should check for the error API returns when Network is not found explicitly.
| } | ||
|
|
||
| switch civoNetwork.Status { | ||
| case NetworkActive: |
There was a problem hiding this comment.
If Network is active- what happens when user updates the NameServers ? Sending ResourceUpToUpdate will mean that Update won't be triggered at all.
| return managed.ExternalCreation{}, nil | ||
| } | ||
| _, err = e.civoClient.CreateNewNetwork(cr.Spec.Name, cr.Spec.CIDRv4, cr.Spec.NameserversV4) | ||
| if err != nil { |
There was a problem hiding this comment.
We should check for the error here and if the error says Network Already exists, we should not return an error.
| return managed.ExternalCreation{}, errors.New(errNotCivoNetwork) | ||
| } | ||
| civoNetwork, err := e.civoClient.GetNetwork(cr.Spec.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
I don't follow- on the first try, isn't this always going to error out as the network won't exist and Civo API would complaint it doesn't exist?
| if !ok { | ||
| return managed.ExternalUpdate{}, errors.New(errNotCivoNetwork) | ||
| } | ||
|
|
There was a problem hiding this comment.
We should check for fields like Nameservers and name/label are changed and update them to desired state.
| } | ||
| cr.SetConditions(xpv1.Deleting()) | ||
| err := e.civoClient.DeleteNetwork(cr.Status.AtProvider.ID) | ||
| return errors.Wrap(err, errDeleteNetwork) |
There was a problem hiding this comment.
If the error here is NetworkNotFound , we should return nil.
| func (c *CivoClient) GetNetwork(id string) (*civogo.Network, error) { | ||
| network, err := c.civoGoClient.GetNetwork(id) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "DatabaseNetworkNotFoundError") { |
There was a problem hiding this comment.
I don't think we should do this here. We should also wrap the error in Error struct that civogo provides and use that but in controllers
There was a problem hiding this comment.
imo its better to handle and return proper errors from the cli.go itself to avoid making contriollers bulky. controllers just have to calls functions in the cli
Signed-off-by: uzair <uzaxirr@gmail.com>
Signed-off-by: uzair <uzaxirr@gmail.com>
Signed-off-by: uzair <uzaxirr@gmail.com>
Description of your changes
Fixes #
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested