Skip to content
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

Fixes: #17923, #17921 - Fix non-null constraint for script execution #17932

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

alehaa
Copy link
Contributor

@alehaa alehaa commented Nov 4, 2024

Fixes: #17921

With c34a0e2, validation of job object fields is enabled, so ScriptJob must not set required fields to empty strings. Changes of this PR revert b18f193 and (hopefully) fix this issue not only for UI views, but for all interactions with scripts.

With c34a0e2, validation of job object fields is enabled, so ScriptJob
must not set required fields to empty strings. This commit reverts
b18f193 and (hopefully) fixes this issue not only for UI views, but for
all interactions with scripts.

Fixes: netbox-community#17923
@alehaa alehaa changed the title Fix non-null constraint for script execution Fixes: #17923, #17921 - Fix non-null constraint for script execution Nov 13, 2024
@miaow2
Copy link
Contributor

miaow2 commented Nov 14, 2024

The proposed fix will set Run Script as the default name for all future job names initiated by custom scripts, which may lead to confusion.
image

I believe a more effective solution would be next:

  1. add name=script.name here to fix running scripts from CLI
job = ScriptJob.enqueue(
    instance=script_obj,
    user=user,
    immediate=True,
    data=data,
    request=NetBoxFakeRequest({
        'META': {},
        'POST': data,
        'GET': {},
        'FILES': {},
        'user': user,
        'path': '',
        'id': uuid.uuid4()
    }),
    commit=commit,
    name=script.name,
)
  1. add name=script.name here to fix running scripts from API
ScriptJob.enqueue(
    instance=script,
    user=request.user,
    data=input_serializer.data['data'],
    request=copy_safe_request(request),
    commit=input_serializer.data['commit'],
    job_timeout=script.python_class.job_timeout,
    schedule_at=input_serializer.validated_data.get('schedule_at'),
    interval=input_serializer.validated_data.get('interval'),
    name=script.name,
)
  1. add name=job.name here to fix jobs scheduling
cls.enqueue(
    instance=job.object,
    user=job.user,
    schedule_at=new_scheduled_time,
    interval=job.interval,
    name=job.name,
    **kwargs,
)
  1. remove name attribute from Meta class here because it's empty and the parent class JobRunner already has classproperty name that will be called if empty name are passed to enqueue method

@alehaa alehaa marked this pull request as draft November 14, 2024 11:51
For recurring jobs, the name must be passed to the next job object when
the job is rescheduled.
@alehaa
Copy link
Contributor Author

alehaa commented Nov 14, 2024

I've added d0ee689 to cover the name being reused when the job is rescheduled. Thanks @miaow2!

For points 1 and 2 mentioned in the comment above, I'd like some feedback from maintainers. Since this is a cosmetic change, there may not be a good or bad design, just a decision based on feeling.

@alehaa alehaa marked this pull request as ready for review November 14, 2024 21:49
@jeremystretch
Copy link
Member

IMO naming the job "run script" is acceptable, because that's what it's doing, and each job retains a relation to its corresponding script.

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

I'm accepting this as a fix for #17921 specifically. Will re-evaluate #17923 separately to see if any additional work is required for that bug. Thanks @alehaa!

@jeremystretch jeremystretch merged commit 09a0e57 into netbox-community:develop Nov 21, 2024
3 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Script Scheduling Broken
3 participants