Skip to content

Commit d82bf31

Browse files
committed
feat: fail fast at preflight when job_definition and job_role are combined
A pre-existing job definition (--aws-batch-job-definition) bakes in its own job role, so combining it with --aws-batch-job-role can never take effect. Reject the combination at the top of _preflight_validate — before _validate_job_role would waste an iam:GetRole call on a role that will be ignored. The per-job check in BatchJobBuilder is retained: a per-rule aws_batch_job_definition resource is not visible at preflight time and can only be caught at submission time.
1 parent 826a5b0 commit d82bf31

2 files changed

Lines changed: 62 additions & 3 deletions

File tree

snakemake_executor_plugin_aws_batch/__init__.py

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -349,10 +349,26 @@ def _preflight_validate(self) -> None:
349349
350350
Best-effort about *uncertainty*: a transient API error or a missing
351351
describe/iam permission degrades to a warning and the workflow proceeds.
352-
Only a confirmed-bad configuration (disabled/invalid queue or compute
353-
environment, ``maxvCpus=0``, or a non-existent job role) raises, before
354-
any job is submitted.
352+
Only a confirmed-bad configuration raises, before any job is submitted:
353+
combining ``--aws-batch-job-role`` with ``--aws-batch-job-definition``
354+
(checked first), a disabled/invalid queue or compute environment,
355+
``maxvCpus=0``, or a non-existent job role.
355356
"""
357+
# Reject --aws-batch-job-role + --aws-batch-job-definition early, before
358+
# _validate_job_role wastes an iam:GetRole call on a role that can never
359+
# be used: a pre-existing job definition bakes in its own role. The
360+
# per-job check in BatchJobBuilder must remain — a per-rule
361+
# aws_batch_job_definition resource is not visible here at preflight time.
362+
if getattr(self.settings, "job_definition", None) and getattr(
363+
self.settings, "job_role", None
364+
):
365+
raise WorkflowError(
366+
"Cannot combine --aws-batch-job-role with "
367+
"--aws-batch-job-definition: the job role is baked into a "
368+
"pre-existing job definition and cannot be overridden per job. "
369+
"Remove --aws-batch-job-role or omit --aws-batch-job-definition."
370+
)
371+
356372
problems = self._queue_problems()
357373
if problems:
358374
raise WorkflowError(

tests/test_preflight.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,49 @@ def test_no_queue_configured_does_not_raise(self):
239239
ex._validate_job_role.assert_called_once()
240240

241241

242+
class TestPreflightJobDefinitionJobRoleCombo:
243+
"""`job_definition` + `job_role` must fail fast, before any queue/role call.
244+
245+
A pre-existing job definition bakes in its own role, so combining it with
246+
`--aws-batch-job-role` can never work. The per-rule resource is caught later
247+
in BatchJobBuilder; this covers the setting-level combo at preflight.
248+
"""
249+
250+
def test_both_set_raises_before_queue_or_role_checks(self):
251+
ex = _executor(
252+
job_definition="my-job-def",
253+
job_role="arn:aws:iam::1:role/my-role",
254+
)
255+
ex._queue_problems = MagicMock()
256+
ex._validate_job_role = MagicMock()
257+
with pytest.raises(WorkflowError) as excinfo:
258+
ex._preflight_validate()
259+
msg = str(excinfo.value)
260+
assert "--aws-batch-job-role" in msg
261+
assert "--aws-batch-job-definition" in msg
262+
# Fail fast: neither the queue describe nor the iam:GetRole check runs.
263+
ex._queue_problems.assert_not_called()
264+
ex._validate_job_role.assert_not_called()
265+
266+
def test_job_definition_only_does_not_raise(self):
267+
ex = _with_queue(
268+
_executor(job_definition="my-job-def"),
269+
{"state": "ENABLED", "status": "VALID", "computeEnvironmentOrder": []},
270+
)
271+
ex._validate_job_role = MagicMock()
272+
ex._preflight_validate() # must not raise
273+
ex._validate_job_role.assert_called_once()
274+
275+
def test_job_role_only_preserves_existing_behaviour(self):
276+
ex = _with_queue(
277+
_executor(job_role="arn:aws:iam::1:role/my-role"),
278+
{"state": "ENABLED", "status": "VALID", "computeEnvironmentOrder": []},
279+
)
280+
ex._validate_job_role = MagicMock()
281+
ex._preflight_validate() # must not raise
282+
ex._validate_job_role.assert_called_once()
283+
284+
242285
class TestValidateJobRole:
243286
def _iam(self, **get_role_kwargs) -> MagicMock:
244287
return MagicMock(get_role=MagicMock(**get_role_kwargs))

0 commit comments

Comments
 (0)