Skip to content

Backend for delete functionality#117

Merged
volcano-sh-bot merged 1 commit intovolcano-sh:mainfrom
Shrutim1505:delete-functionality-backend
May 23, 2025
Merged

Backend for delete functionality#117
volcano-sh-bot merged 1 commit intovolcano-sh:mainfrom
Shrutim1505:delete-functionality-backend

Conversation

@Shrutim1505
Copy link
Member

Description:
This PR introduces the backend implementation for the delete functionality. It includes the necessary route, controller logic, and service integration to handle delete operations securely and efficiently.

Changes Made:

Added DELETE API endpoint in the backend

Implemented controller and service logic for delete operation

Integrated appropriate validations and error handling

Updated documentation (if applicable)

Testing:

Verified delete functionality with valid and invalid inputs

Ensured proper status codes and messages are returned

Confirmed no unintended side effects on other endpoints

@Shrutim1505
Copy link
Member Author

/assign @Monokaix
There were some mistakes in #88
I am closing #88 please consider this one

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch 2 times, most recently from 54f9943 to adc2e9d Compare May 20, 2025 15:38
@Shrutim1505
Copy link
Member Author

image

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch 2 times, most recently from 32db801 to 366ebf0 Compare May 20, 2025 16:34
@JesseStutler
Copy link
Member

image
I tried to construct a 403 request (no clusterrole), but the error message was incorrect. I checked the error and it was a 502 error.

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from 5cddc9f to bbfc5ed Compare May 21, 2025 07:20
@Shrutim1505
Copy link
Member Author

Shrutim1505 commented May 21, 2025

i am not getting this error @JesseStutler , but i modified error handling part, can u check it once more??

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from ab1050b to 6472e41 Compare May 21, 2025 16:57
@Shrutim1505
Copy link
Member Author

@Monokaix @JesseStutler

Screen.Recording.2025-05-21.223457.mp4

@JesseStutler
Copy link
Member

Sorry my previous mistake, I think we still need to wrap the error message returned from the kube-apiserver, directly return the whole body is not enough, like this pic:
image
We can show the message : Can not delete "queue-a", error message: queues.scheduling.volcano.sh "queue-a" is forbidden: User "system:serviceaccount:volcano-system:volcano-dashboard" cannot delete resource "queues" in API group "scheduling.volcano.sh" at the cluster scope,Note that you need to delete those \\ @Shrutim1505

@JesseStutler
Copy link
Member

@Shrutim1505 BTW, please clean your commit message:
image

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from 6472e41 to 5d6062e Compare May 22, 2025 05:11
@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from 5d6062e to 51e2287 Compare May 22, 2025 05:24
@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from b8db733 to 5c1881c Compare May 22, 2025 05:51
`Failed to delete queue (${response.status})`,
);
} else {
switch (response.status) {
Copy link
Contributor

Choose a reason for hiding this comment

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

However, the current approach can be a bit verbose and hard to maintain as the app scales.

} catch (err) {
const statusCode = err?.statusCode || err?.response?.statusCode;

if (statusCode === 404) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from 7fe0470 to c9f5c99 Compare May 22, 2025 12:25
@Shrutim1505
Copy link
Member Author

@JesseStutler @Monokaix please check it

Copy link
Member

@JesseStutler JesseStutler left a comment

Choose a reason for hiding this comment

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

image
Still incorrect, where is the error message got from kube-apiserver returned status...?

@Shrutim1505
Copy link
Member Author

image
check it now @JesseStutler

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from ef6d1af to 24c7c25 Compare May 23, 2025 05:10
@JesseStutler
Copy link
Member

Now it's OK, please clean your commit message, thanks

@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from 24c7c25 to 8f69723 Compare May 23, 2025 07:29
Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>
@Shrutim1505 Shrutim1505 force-pushed the delete-functionality-backend branch from 8f69723 to 31c5d7c Compare May 23, 2025 07:34
@Shrutim1505
Copy link
Member Author

@JesseStutler please check

@JesseStutler
Copy link
Member

/lgtm

@Monokaix
Copy link
Member

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot merged commit 32f205f into volcano-sh:main May 23, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants