Skip to content

Make sure we check for executable's existence before submitting job#657

Merged
shreyb merged 2 commits into
fermitools:masterfrom
shreyb:issue-631
Jan 10, 2026
Merged

Make sure we check for executable's existence before submitting job#657
shreyb merged 2 commits into
fermitools:masterfrom
shreyb:issue-631

Conversation

@shreyb
Copy link
Copy Markdown
Collaborator

@shreyb shreyb commented Jan 5, 2026

Closes #631

This PR upgrades the executable argument in the jobsub get_parser parser to use a custom Action that does two things:

  1. Check that the executable argument is given with the correct URI (file://)
  2. Check that the argument value for the executable actually exists.

There are added accompanying unit tests, as well as a couple of updates to old tests so that they work with this new restriction.

All unit tests pass, and further testing information is available in comments of #631.

…tom action that also checks for executable's existence
@shreyb shreyb added this to the 1.13 milestone Jan 5, 2026
@shreyb shreyb requested a review from Copilot January 5, 2026 17:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the executable argument validation in jobsub by replacing the simple type validation function with a comprehensive custom argparse Action class. The new implementation checks both that the executable argument uses the correct file:// URI format and that the file actually exists before job submission.

Key Changes

  • Replaced verify_executable_starts_with_file_colon function with CheckExecutable custom action class that performs both URI format and file existence validation
  • Updated the executable argument definition to use the new action with type=str and action=CheckExecutable
  • Added three new unit tests covering valid executables, invalid URI format, and non-existent files
  • Updated two existing tests to provide valid executable arguments now that validation is enforced

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/get_parser.py Removed old validation function and added CheckExecutable action class that validates URI format and file existence; updated executable argument to use the new action
tests/test_get_parser_unit.py Added fixture and three tests for executable validation; updated existing tests to pass valid executable paths that comply with new validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_get_parser_unit.py Outdated
@shreyb shreyb requested a review from vitodb January 5, 2026 17:12
Copy link
Copy Markdown
Contributor

@vitodb vitodb left a comment

Choose a reason for hiding this comment

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

This looks good

@shreyb shreyb merged commit 2df7cb0 into fermitools:master Jan 10, 2026
4 checks passed
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.

Upgrade executable verification action to check for file existence (file://)

3 participants