-
Notifications
You must be signed in to change notification settings - Fork 31
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
Runtimejob API ModelView #1191
Runtimejob API ModelView #1191
Conversation
Signed-off-by: Akihiko Kuroda <[email protected]>
@akihikokuroda would adding the additional API change this PR? or would it be done separately? |
@psschwei I'll do it separately. |
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
I still feel a bit iffy on the django framework, so I'd like either @IceKhan13 or @Tansito to review also
@psschwei Thanks for review! |
gateway/api/v1/views.py
Outdated
RuntimeJob view set first version. Use RuntomeJobSerializer V1. | ||
""" | ||
|
||
queryset = RuntimeJob.objects.all() |
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 will select all runtime jobs, even not authored by user :)
gateway/api/v1/views.py
Outdated
|
||
queryset = RuntimeJob.objects.all() | ||
serializer_class = v1_serializers.RuntimeJobSerializer | ||
permission_classes = [permissions.IsAuthenticated] |
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.
Since IsOwner
changed, we can add it now here
gateway/api/views.py
Outdated
return self.serializer_class | ||
|
||
def get_queryset(self): | ||
return RuntimeJob.objects.all() |
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.
same as above. We need to select only authored runtime jobs.
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.
Looks good. Couple of tweaks and we are good to go!
@IceKhan13 Thanks for review. I updated the code and also updated the test to make sure it works. |
@IceKhan13 WDYT about additional APIs Adding runtime job to the job:
list the runtime jobs created in the job:
I can create another PR if they are good. |
sounds like a good idea! Let's create a separate PR for this APIs Thank 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.
Looks great! Thank you @akihikokuroda !
Summary
This PR adds ModelView API for RuntimeJob model
Details and comments
With this, the get, list and delete operations are easy and useful but the create operation is quite difficult to issue. Form the usage of the RuntimeJob view, it may be better to have the following additional API in the
Job
./api/v1/jobs/{job id}/add_runtimejob
and
/api/v1/jobs/{job id}/list_runtimejob