-
Notifications
You must be signed in to change notification settings - Fork 34
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
Initiating Nautobot jobs, next-2.0 rebase #270
Conversation
Update init_jobs for new pattern, tested and working Add kwargs to init_jobs, untested but looks good Add get_jobs which returns all Nautobot jobs viewable to user Add filter_jobs which returns filtered set of Nautobot jobs viewable to user
…loses nautobot#223 Run black formatter
…be exported for jobs.
Update markdown to URL
Got some edge cases left I think, but pretty much everything but making CI happy is left |
I left a few TODO entries, but nothing is a blocker I think |
Not sure if making pylint happy for generator is mandatory for merge or if we can add an ignore for this case |
nautobot_chatops/workers/nautobot.py
Outdated
profile = True | ||
|
||
# Get instance of the user who will run the job | ||
user = get_user_model() |
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.
You do not need to get the user model, or get the user, as dispatcher.user is already a user instance.
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.
thanks, pushed d7e4ccc
nautobot_chatops/workers/nautobot.py
Outdated
return (CommandStatusChoices.STATUS_FAILED, f'Job "{job_name}" failed to initiate. Result: {job_result.result}') | ||
|
||
# TODO: need base-domain, this yields: /extras/job-results/<job_id>/ | ||
job_url = job_result.get_absolute_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.
We can use dispatcher.context['request_schema']
and dispatcher.context['request_host']
to get the base-domain. But this won't work in Slack Socket Mode. I added the SLACK_SOCKET_STATIC_HOST
to solve that issue for static urls, but since static
could point to S3, we would need another solution here.
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.
noted, will explore this
nautobot_chatops/workers/nautobot.py
Outdated
|
||
job_class_path = job_model.class_path | ||
|
||
# TODO: Check if json_args keys are valid for this job model |
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.
It might also be a good idea to ensure the user has permissions to run the job.
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.
does the restrict() a few lines up do this? I can test to confirm
job_model = Job.objects.restrict(dispatcher.user, "view").get(name=job_name)```
Thanks for the feedback, Ill get this updated! |
My current action/to-do items for this PR:
|
status update on TODO item -> "have a job's user-input widgets prompt for chat user input" |
Bug: job forms with only 1-item
prompt_for_job added to init-job subcommand removed testing code blocks todo: bug: single job forms wont submit properly with multi_input_dialog notes: merge init-job and init-job-form seems best
…attern Added wait for job to initiate using refresh_from_db Corrected the initiated job url hyperlink message
I removed this helper function, noting in case we want to re-add (eg: enable_job subcommand would use this) def enable_job(module, name, source="local"):
"""
Helper function to look up a job by class and job model and enable it.
Args:
module (str): Job module name
name (str): Job class name
source (str): Job grouping (default: "local")
"""
job_class = get_job(f"{module}.{name}")
job_model = Job.objects.get(module_name=module, job_class_name=name) # .restrict() may be preferred if we pass user
if not job_model.enabled:
job_model.enabled = True
job_model.validated_save()
return |
Noting some things for the current status of this PR:
Fixed items:
|
Follow existing pylint ignore pattern on repo
) | ||
|
||
# Wait on the job to finish | ||
while job_result.status not in JobResultStatusChoices.READY_STATES: |
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.
We may want something to drop out of this loop after enough time?
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.
We may want something to drop out of this loop after enough time?
Addressed in 7470ec8
nautobot_chatops/workers/nautobot.py
Outdated
|
||
|
||
@subcommand_of("nautobot") | ||
def init_job(dispatcher, *args, job_name: str = "", json_string_kwargs: str = ""): # pylint: disable=too-many-locals |
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.
Would this make more sense to be named start_job
or maybe launch_job
? And then as I put that into suggestion, I then looked at Nautobot itself. Probably run_job
would make sense to line up with the Run Job button.
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.
run_job
should be fine for the namespace. Initially I did not use run_job
namespace since there was a similar import nautobot.extras.jobs import run_job
, but this has been replaced by enqueue_job
anyways in the PR
Rename `init_job_form`subcommand to `run_job_form`
nautobot_chatops/workers/nautobot.py
Outdated
|
||
if job_result and job_result.status == "FAILURE": | ||
dispatcher.send_error(f"The requested job {job_name} failed to initiate. Result: {job_result.result}") | ||
return (CommandStatusChoices.STATUS_FAILED, f'Job "{job_name}" failed to initiate. Result: {job_result.result}') |
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.
This is checking if the task failed, but then sending a message that the task failed to initiate. The task was initiated but failed.
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.
Good point, I adjusted the phrasing in 875ca64, but let me know if you have anything specific in mind maybe
Awesome work @MeganerdDev! The only thing I'd note is the addition of some documentation in the |
Closes: #223
#224 closed in favor of this rebase
What's Changed
Adds the following nautobot subcommands:
init_job()
asinit-job
init_job
subccomand which initiates a Nautobot job by job namekwargs
JSON-string dictionary to init_jobs (untested but looks good)get_jobs()
asget-jobs
get_jobs
which returns all Nautobot jobs viewable to userkwargs
JSON-string list to get_jobs export header items (tested as working)filter_jobs()
asfilter-jobs
filter_jobs
which returns filtered set of Nautobot jobs viewable to user/nautobot filter-jobs "enabled"
or/nautobot filter-jobs "enabled,installed"
/nautobot filter-jobs '{"enabled": true}'
Remaining work
init_job
should presen thet job form itemsinit_job
method we can pass kwargs as JSON-string (untested)TODO
Screenshots
/nautobot init-job
/nautobot init-jobs for job that is not enabled
if there is a failed job, we will return the job_result.result message
/nautobot get-jobs
by default we will get some head items
/nautobot get-jobs '["id", "name", "installed", "tags"]'
we can specify the header items we want with a JSON formatted list
if there is a bad key, eg:
/nautobot get-jobs '["id", "name", "invalid key demo"]'
/nautobot filter-jobs "enabled"
Notes