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

feat(cli): Add 'deadline worker delete' command #598

Open
wants to merge 1 commit into
base: mainline
Choose a base branch
from

Conversation

RaiaN
Copy link

@RaiaN RaiaN commented Feb 10, 2025

Fixes:

What was the problem/requirement? (What/Why)

Customer wanted to delete a worker

What was the solution? (How)

Use deadline client API to delete a worker by id

What is the impact of this change?

Customer is happy

How was this change tested?

No testing required, just simple API call

Was this change documented?

Yes

Does this PR introduce new dependencies?

No

Is this a breaking change?

No, it is merely a change, just another API call

Does this change impact security?

No

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RaiaN RaiaN requested a review from a team as a code owner February 10, 2025 21:09
Copy link

Copy link
Contributor

@epmog epmog left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR! I just have the one comment about adding a prompt (and a way to automatically accept that prompt).

Additionally, to get the PR merged the github actions will have to pass. You'll notice that DCO and Semantic PR are failing. From the contributor guidelines we need you to sign your commit and have the message follow the conventional commit syntax. Here's how you can do that given some staged files:

git commit -s -m "feat(cli): add 'deadline worker delete' command"

Comment on lines +88 to +90
@click.option("--worker-id", help="The worker to delete.", required=True)
@_handle_error
def worker_delete(fleet_id, worker_id, **args):
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to deadline cancel job we should add a confirmation (defaults to no) and a --yes option in here before a user performs a destructive action.

@click.option(
"--yes",
is_flag=True,
help="Skip any confirmation prompts",
)
@_handle_error
def job_cancel(mark_as: str, yes: bool, **args):

Something like:

Deleting a worker will remove the resource and it will no longer be visible in the AWS Deadline Cloud service. Are you sure you wish to proceed?

Here's how the cancel commands prompts the user

# We explicitly require a yes/no response, as this is an operation that will interrupt the work in progress
# on their job.
if not click.confirm(
cancel_message,
default=None,
):
click.echo("Job not canceled.")
sys.exit(1)

We can ignore adding the auto-accept for now. But we should change the help text for the option to say "confirms the deletion of the worker".

@epmog epmog changed the title Add command to delete a worker feat(cli): Add 'deadline worker delete' command Feb 11, 2025
@RaiaN
Copy link
Author

RaiaN commented Feb 17, 2025

Thank you, I will fix PR

@epmog epmog added enhancement New feature or request response-requested A response from the contributor has been requested. labels Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request response-requested A response from the contributor has been requested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: How to purge old workers (status: Not responding)
2 participants