Skip to content

Remove the 'all-priority-classes' flag from armadactl commands - #5135

Open
JamesMurkin wants to merge 2 commits into
masterfrom
simplify_node_operations
Open

Remove the 'all-priority-classes' flag from armadactl commands#5135
JamesMurkin wants to merge 2 commits into
masterfrom
simplify_node_operations

Conversation

@JamesMurkin

@JamesMurkin JamesMurkin commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

For cancel/preempt queue/node/executor, we now simply default to all priority classes unless a priority class filter is provided

The 'all-priority-classes' was cumbersome and users typically always wanted 'all-priority-classes'

Making it default aligns the command with user expectation and makes it easier to use, while keeping the optional filter of priority classes if users need it

I've also reworked the tests - so we don't override RunE etc and let that use the real production code

  • This means the tests now run the actual production code and just assert the outputs, which I think is much more robust

For cancel/preempt queue/node/executor, we now simply default to all priority classes unless a priority class filter is provided

The 'all-priority-classes' was cumbersome and users typically always wanted 'all-priority-classes'

Making it default aligns the command with user expectation and makes it easier to use, while keeping the optional filter of priority classes if users need it

Signed-off-by: JamesMurkin <jamesmurkin@hotmail.com>
@JamesMurkin
JamesMurkin marked this pull request as ready for review August 27, 2026 11:21
@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR removes the all-priority-classes flag from cancel and preempt commands and treats an omitted priority-class filter as selecting every class.

  • Updates executor, node, and queue command defaults and help text.
  • Injects the application dependency into command constructors to support production-path testing.
  • Reworks tests to execute real command handlers through API argument construction while faking only external API calls.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
cmd/armadactl/cmd/cancel.go Removes the explicit all-priority-classes flag and defaults empty priority-class selections to all classes for cancellation commands.
cmd/armadactl/cmd/cancel_test.go Exercises real cancellation command handlers and verifies their arguments at the API boundary.
cmd/armadactl/cmd/preempt.go Applies the same default-all priority-class behavior and injectable command construction to preemption commands.
cmd/armadactl/cmd/preempt_test.go Adds shared API-boundary fakes and verifies real preemption command parsing and request construction.

Reviews (2): Last reviewed commit: "More robust tests" | Re-trigger Greptile

Comment thread cmd/armadactl/cmd/cancel_test.go Outdated
Signed-off-by: JamesMurkin <jamesmurkin@hotmail.com>
@mergify

mergify Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

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