e2b add volume api#580
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ZhaoQing7892
force-pushed
the
add_e2b_volume
branch
from
5f274f2 to
1f06208
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #580 +/- ##
==========================================
- Coverage 79.85% 79.75% -0.11%
==========================================
Files 202 205 +3
Lines 14796 15226 +430
==========================================
+ Hits 11816 12143 +327
- Misses 2551 2643 +92
- Partials 429 440 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ZhaoQing7892
force-pushed
the
add_e2b_volume
branch
from
1f06208 to
7d9d70f
Compare
ZhaoQing7892
force-pushed
the
add_e2b_volume
branch
from
7d9d70f to
376fef8
Compare
| }, | ||
| } | ||
|
|
||
| err := i.Cache.GetClient().Create(ctx, pvc) |
There was a problem hiding this comment.
plz add PVC create and list role and rolebinding
|
|
||
| // If PVC is still Pending, check Events for provisioning failures | ||
| if pvc.Status.Phase == corev1.ClaimPending { | ||
| events := &corev1.EventList{} |
There was a problem hiding this comment.
it is bad practice for controller to rely on k8s event for handling, consider using pvc conditions instead
There was a problem hiding this comment.
Done. Removed Event queries and switched to checking pvc.Status.Conditions for failure detection.
| // validateStorageClass checks if the StorageClass exists and uses Immediate binding mode. | ||
| func (i *Infra) validateStorageClass(ctx context.Context, storageClassName string) error { | ||
| // StorageClass is a cluster-scoped resource, no namespace needed | ||
| sc := &storagev1.StorageClass{} |
There was a problem hiding this comment.
plz add storage class role and rolebinding
|
|
||
| // Volume management endpoints | ||
| RegisterE2BRoute(sc.mux, http.MethodPost, "/volumes", sc.CreateVolume, sc.CheckApiKey) | ||
| RegisterE2BRoute(sc.mux, http.MethodGet, "/volumes", sc.ListVolumes, sc.CheckApiKey) |
There was a problem hiding this comment.
plz modify CheckApiKey, so that we can ensure that the volume is owned by the user.
| ) | ||
|
|
||
| // CreateVolume creates a new PersistentVolumeClaim for the user | ||
| func (i *Infra) CreateVolume(ctx context.Context, opts infra.CreateVolumeOptions) (*infra.VolumeInfo, error) { |
There was a problem hiding this comment.
this file has zero UT coverage, plz add UT
| ) | ||
|
|
||
| const ( | ||
| DefaultWaitBoundPVCTimeout = 60 * time.Second |
There was a problem hiding this comment.
DefaultWaitBoundPVCTimeout is in a separate const () block from the other constants. It could be in the same block.
| } | ||
|
|
||
| err := i.Cache.GetClient().Create(ctx, pvc) | ||
| if err != nil { |
There was a problem hiding this comment.
plz handle already exist error
| func validateVolumeRequest(request models.NewVolumeRequest) error { | ||
| if request.Name == "" { | ||
| return fmt.Errorf("name is required") | ||
| } |
There was a problem hiding this comment.
plz Add DNS-1123 label validation for the volume name to catch invalid names before they reach the API server.
There was a problem hiding this comment.
Done. Added DNS-1123 label validation in validateCreateVolumeOptions()
| }) | ||
| if err != nil { | ||
| log.Error(err, "Failed to get volume", "volumeID", volumeID, "namespace", namespace) | ||
| if apierrors.IsNotFound(err) || managererrors.GetErrCode(err) == managererrors.ErrorNotFound { |
There was a problem hiding this comment.
GetVolume never return apierrors.IsNotFound error , consider remove the condition, and only checks for managererrors.ErrorNotFound
There was a problem hiding this comment.
Done. Removed the apierrors.IsNotFound check
3 participants
No description provided.