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

Added Support for Civo Object Store #54

Closed
wants to merge 15 commits into from
Closed

Conversation

uzaxirr
Copy link
Collaborator

@uzaxirr uzaxirr commented Mar 7, 2024

Description of your changes

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

@uzaxirr uzaxirr changed the title Object Store Added Support for Civo Object Store Mar 9, 2024
@uzaxirr uzaxirr self-assigned this Mar 9, 2024
@uzaxirr uzaxirr marked this pull request as ready for review March 9, 2024 05:49
@uzaxirr uzaxirr requested a review from RealHarshThakur March 9, 2024 06:29
return managed.ExternalObservation{ResourceExists: true}, err
}
}
_, err = e.Update(ctx, mg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should only update if the value of secret's don't match the access key id , etc we got from civo api.

return err
}
objectStore, err := e.civoClient.GetObjectStoreByName(os.Spec.Name)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function is not idempotent. If the resource does not exist, we should consider it deleted.

apis/civo/objectstore/v1alpha1/doc.go Outdated Show resolved Hide resolved
apis/civo/objectstore/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/civo/objectstore/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/civo/objectstore/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/civo/objectstore/v1alpha1/types.go Show resolved Hide resolved
// +kubebuilder:resource:shortName="cos"
// +kubebuilder:printcolumn:name="State",type="string",JSONPath=".status.atProvider.status",description="State of the Bucket"
// +kubebuilder:printcolumn:name="Bucket",type="string",JSONPath=".spec.name",description="Name of the Bucket which can be used against S3 API"
// +kubebuilder:printcolumn:name="Size",type="string",JSONPath=".spec.maxSize",description="Size of the Bucket in GB"
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 it might be better to have used capacity here rather than requested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again how should we get the used capacity?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an endpoint at /v2/objectstore/id/stats.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check if we can display a percentage here? Would be nice to show utilised percentage.
Basically, .status.usedCapacity/.status.maxSize*100

Copy link
Collaborator

Choose a reason for hiding this comment

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

If not, rather than displaying usedCapacity, let's have utilisedPercentage in status and drop usedCapacity

UsedCapacity string `json:"usedCapacity"`

// Status of the Object Store.
Status CivoObjectStoreStatus `json:"status,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop this field.

Status CivoObjectStoreStatus `json:"status,omitempty"`

// Region where the Object Store is located
Region string `json:"region,omitempty"`
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 this? Isn't it implicit since they've the provider configured to single region?

@uzaxirr uzaxirr force-pushed the objectstore-support branch from 9c00fea to 1af7164 Compare March 19, 2024 16:32
@uzaxirr uzaxirr closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants