Skip to content

fix: honor --s3-root-uri and --conflict-resolution in attachment CLI #1292

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.

Sign up for GitHub

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

Open
crowecawcaw wants to merge 7 commits into
base: mainline
Choose a base branch
from
Open

fix: honor --s3-root-uri and --conflict-resolution in attachment CLI #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
715b871
fix: consume --conflict-resolution in config-defaults workflow
crowecawcaw Jul 21, 2026
ce70fcd
fix: honor --s3-root-uri independently of --profile
crowecawcaw Jul 21, 2026
fa3bbe8
fix: allow explicit --s3-root-uri when queue lacks attachment settings
crowecawcaw Jul 21, 2026
1eea755
Merge branch 'mainline' into review-fix/attachment-args
crowecawcaw Jul 22, 2026
0a2af63
test: cover explicit --s3-root-uri when queue lacks attachment settings
crowecawcaw Jul 22, 2026
1dfb445
test: cover conflict-resolution threading and missing-settings failur…
crowecawcaw Jul 22, 2026
e937dd0
test: verify explicit --s3-root-uri end-to-end against moto S3
crowecawcaw Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/deadline/client/cli/_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,15 @@ def _apply_cli_options_to_config(
)
else:
# Remove the standard option names from the args list
for name in ["profile", "farm_id", "region", "queue_id", "job_id", "storage_profile_id"]:
for name in [
"profile",
"farm_id",
"region",
"queue_id",
"job_id",
"storage_profile_id",
"conflict_resolution",
]:
args.pop(name, None)

# Check that the required options have values, auto-selecting if only one exists
Expand Down
24 changes: 18 additions & 6 deletions src/deadline/client/cli/_groups/attachment_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,16 @@ def attachment_download(
queue_id=queue_id,
session=boto3_session,
).jobAttachmentSettings
if not s3_settings:
raise MissingJobAttachmentSettingsError(f"Queue {queue_id} has no attachment settings")

s3_root_uri = s3_settings.to_s3_root_uri()
# Only fall back to the queue's S3 settings when the caller did not provide an
# explicit --s3-root-uri. An explicitly-supplied value must always be honored,
# even when the queue has no attachment settings of its own.
if not s3_root_uri:

@github-actions github-actions Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new comment says an explicit --s3-root-uri must be honored "even when the queue has no attachment settings of its own", but get_queue(...) (lines 126-130) is still called unconditionally before this check. So when a caller supplies --s3-root-uri precisely to operate without relying on queue settings, they still incur a GetQueue call — and if that call fails (e.g. AccessDenied, or the queue does not exist), the command errors out before ever reaching this branch, defeating the intent. Consider moving the get_queue(...) lookup inside the if not s3_root_uri: block so it is only performed when the fallback is actually needed.

if not s3_settings:
raise MissingJobAttachmentSettingsError(
f"Queue {queue_id} has no attachment settings"
)
s3_root_uri = s3_settings.to_s3_root_uri()
Comment thread
crowecawcaw marked this conversation as resolved.

deadline_client = get_session_client(boto3_session, "deadline", region=region)
boto3_session = api.get_queue_user_boto3_session(deadline=deadline_client, config=config)
Expand Down Expand Up @@ -240,10 +246,16 @@ def attachment_upload(
queue_id=queue_id,
session=boto3_session,
).jobAttachmentSettings
if not s3_settings:
raise MissingJobAttachmentSettingsError(f"Queue {queue_id} has no attachment settings")

s3_root_uri = s3_settings.to_s3_root_uri()
# Only fall back to the queue's S3 settings when the caller did not provide an
# explicit --s3-root-uri. An explicitly-supplied value must always be honored,
# even when the queue has no attachment settings of its own.
if not s3_root_uri:

@github-actions github-actions Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same as in attachment_download: get_queue(...) (lines 244-248) is still called unconditionally, so an explicit --s3-root-uri does not actually let a caller bypass the queue lookup. If GetQueue fails (AccessDenied, missing queue), upload errors out before reaching this fallback branch, contradicting the "must always be honored, even when the queue has no attachment settings" comment. Move the get_queue(...) call inside if not s3_root_uri:.

if not s3_settings:
raise MissingJobAttachmentSettingsError(
f"Queue {queue_id} has no attachment settings"
)
s3_root_uri = s3_settings.to_s3_root_uri()
Comment thread
crowecawcaw marked this conversation as resolved.

deadline_client = get_session_client(boto3_session, "deadline", region=region)
boto3_session = api.get_queue_user_boto3_session(deadline=deadline_client, config=config)
Expand Down
Loading
Loading