-
Notifications
You must be signed in to change notification settings - Fork 75
Integrate cli commands with job scheduling system #125
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.
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
base: master
Are you sure you want to change the base?
Conversation
…onality Create test
| cron = croniter(schedule) # Validate the cron schedule | ||
| except CroniterBadCronError: | ||
| raise argparse.ArgumentTypeError(f"Invalid cron schedule: '{schedule}'. " | ||
| "Make sure it follows the cron syntax format:\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this throw a error since its essentially a bunch of separate strings instead of one big one? I think you're looking for a block string with ''' abcdefg '''
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems okay to me when I tested, seems like croniter is built to handle this
vast.py
Outdated
|
|
||
| if (args.schedule): | ||
| cli_command = "change bid" | ||
| api_endpoint = "/api/v0" + "/instances/bid_price/{id}/".format(id=args.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not reuse the url variable that is equivalent to api_endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url also includes hostname, port maybe, etc., which we don't want to store in the scheduled_jobs table
vast.py
Outdated
| print(filtered_text) | ||
| if (args.schedule): | ||
| cli_command = "execute" | ||
| api_endpoint = "/api/v0" + "/instances/command/{id}/".format(id=args.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah still the same url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't want hostname:port, etc. included
vast.py
Outdated
| print("Rebooting instance {args.ID}.".format(**(locals()))); | ||
| if (args.schedule): | ||
| cli_command = "reboot instance" | ||
| api_endpoint = "/api/v0" + "/instances/reboot/{id}/".format(id=args.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and again lol- perhaps just api_endpoint = url if you wanna keep your descriptive variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't want hostname:port, etc. included
| "request_body": req_json | ||
| } | ||
| # Send a POST request | ||
| response = requests.post(schedule_job_url, headers=headers, json=request_body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ask for request_method if only POST requests are called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scheduled_jobs table is intended to store GET, POST, PUT, or DELETE requests. but call to scheduled_jobs api endpoint will be post if inserting a new record and PUT if updating
| parser.add_argument("--explain", action="store_true", help="output verbose explanation of mapping of CLI calls to HTTPS API endpoints") | ||
| parser.add_argument("--schedule", help="try to schedule a command to run every x mins, hours, etc. by passing in time interval in cron syntax to --schedule option. Can also choose to have --start_time and --end_time options with valid values. For ex. --schedule \"0 */2 * * *\"") | ||
| parser.add_argument("--start_time", help="the start time for your scheduled job in millis since unix epoch. Default will be current time. For ex. --start_time 1728510298144", default=(time.time() * 1000)) | ||
| parser.add_argument("--end_time", help="the end time for your scheduled job in millis since unix epoch. Default will be 7 days from now. For ex. --end_time 1729115232881", default=(time.time() * 1000 + 7 * 24 * 60 * 60 * 1000)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yknow you can enforce type with argparse arguments by just adding type=int
also if you want it so that the user can choose --schedule OR (start_time and end_time) I recommend checking out argparse's mutually exclusive groups :D up to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not mutually exclusive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Enabling these cli commands to be executed in scheduled job fashion: cloud copy, change bid, reboot instance, execute