Add job_name argument to cron wrapper script#11589
Add job_name argument to cron wrapper script#11589mekarpeles merged 2 commits intointernetarchive:masterfrom
job_name argument to cron wrapper script#11589Conversation
jimchamp
added
the
Needs: Special Deploy
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR refactors the cron_wrapper.py script to accept an explicit job_name argument instead of deriving it from the wrapped script's path. This change provides more flexibility in naming monitored jobs independently of their file paths, but introduces a breaking change to the CLI interface.
Key Changes
- Adds
job_nameas a required positional argument to the CLI - Removes the
_get_job_name()method that parsed job names from script paths - Updates
MonitoredJob.__init__()to acceptjob_nameas a parameter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _parser.add_argument("job_name", help="Name of the job to be monitored") | ||
| _parser.add_argument( | ||
| "script", help="Path to script that will be wrapped and monitored" |
There was a problem hiding this comment.
Adding a positional argument before the existing 'script' argument creates a breaking change to the CLI interface. All existing cron jobs calling this wrapper will need to be updated to include the job_name argument. Consider making this an optional flag (e.g., '--job-name') or adding it after 'script' to minimize impact, or ensure all call sites are updated simultaneously.
| ) | ||
| self._setup_sentry(sentry_cfg.get("dsn", "")) | ||
| self.job_name = self._get_job_name() | ||
| self.job_name = job_name |
There was a problem hiding this comment.
The job_name parameter should be validated to ensure it doesn't contain characters that would be problematic for StatsD metric names (e.g., dots are used as metric delimiters, spaces could cause issues). The job_name is used directly in metric names like 'cron.{self.job_name}.start', so invalid characters could lead to metric reporting issues.
| sentry_cfg = dict(config["sentry"]) if config.has_section("sentry") else None | ||
| statsd_cfg = dict(config["statsd"]) if config.has_section("statsd") else None | ||
| command = [args.script] + args.script_args | ||
| job_name = args.job_name |
There was a problem hiding this comment.
This intermediate variable assignment is unnecessary. The value can be passed directly to MonitoredJob on line 79 as 'args.job_name', which would be more concise and consistent with how 'command' is constructed on the previous line.
Updates
cron_wrapperscript to accept a newjob_nameargument. Removes method that derives job name from the wrapped script's path.Special Deployment Procedures
The new argument is required, so our cron file will need to be updated before this is deployed.
Technical
Testing
Screenshot
Stakeholders