add a patch to support webhooks #98

edyst-tech · 2019-08-29T07:05:05Z

No description provided.

hermanzdosilovic · 2019-09-01T01:10:28Z

Dockerfile

@@ -15,7 +15,7 @@ ENV PATH "/usr/local/ruby-2.3.3/bin:/opt/.gem/bin:$PATH"
 ENV GEM_HOME "/opt/.gem/"
 RUN echo "gem: --no-document" > /root/.gemrc && \
    gem install \
-      rails:5.0.0 \
+      rails:5.1.7 \


Why is Rails updated?

5.1.x has support for permitting arbitrary hashes.
rails/rails@e86524c

hermanzdosilovic

This is a good start for #33 but we need to get back on the whiteboard for this to be closed.

I would suggest that you answer few questions:

Why do you need multiple callback URLs?
How can we log the success of calling this URL?
How can user see the status of calling this URL?
Does the call_webhooks function need to be in the IsolateJob or can it be in some other job so that we don't spend time on sending the data. Maybe call_webhooks can be called by some other non priority worker that just sends data to the workers. After all, we want the code to be executed as fast as possible and we want to free workers as soon as we can so they can run some other code.
What data do we need to send to the callback? Can user decide what it needs?

Think about these questions and try answering them so we can iterate on this feature.

Thanks for your contribution.

hermanzdosilovic · 2019-09-01T01:11:33Z

.gitignore

@@ -6,3 +6,4 @@
 .byebug_history
 coverage/
 judge0-api.conf
+.vscode/


Entries in this file should be sorted.

Noted, will fix

hermanzdosilovic · 2019-09-01T01:12:03Z

.rake_tasks~

@@ -0,0 +1,49 @@
+about


Why this file exists?

hermanzdosilovic · 2019-09-01T01:12:33Z

Gemfile

@@ -26,3 +26,5 @@ group :development, :test do
  gem 'pry-byebug', '~> 3.4'
  gem 'rspec-rails', '~> 3.5'
 end
+
+gem "httparty", "~> 0.17.0"


This should be added alongside with other gems. Sorted again.

Noted, will fix

hermanzdosilovic · 2019-09-01T01:13:33Z

app/controllers/submissions_controller.rb

    )
-
+    


Spaces are unnecessary.

Noted, will fix

hermanzdosilovic · 2019-09-01T01:14:52Z

app/jobs/isolate_job.rb

+        "postdata": item["postdata"],
+        "submission": submission.to_json
+      }
+      response = HTTParty.post(


What happens if this doesn't succeed? Don't we want to log that in our database somehow?

True, we definitely should. We shall start with logging errors/warnings here.

hermanzdosilovic · 2019-09-01T01:15:46Z

app/jobs/isolate_job.rb

@@ -20,6 +22,7 @@ def perform(submission)
      write
      if compile == :failure
        clean
+        call_webhooks(submission)


You don't need to pass submission as an argument because it is available as member of this class.

Noted, will fix

hermanzdosilovic · 2019-09-01T01:16:24Z

app/jobs/isolate_job.rb

@@ -196,4 +201,20 @@ def strip_output(output)
  rescue ArgumentError
    return output
  end
-end
+
+  def call_webhooks(submission)


Function signature could be just def call_webhooks because submission is available as class member.

Noted, will fix

hermanzdosilovic · 2019-09-01T01:17:16Z

app/controllers/submissions_controller.rb

@@ -71,9 +71,10 @@ def submission_params
      :max_processes_and_or_threads,
      :enable_per_process_and_thread_time_limit,
      :enable_per_process_and_thread_memory_limit,
-      :max_file_size
+      :max_file_size,
+      { webhooks: [ :url, postdata: {} ] },


Why does the user need to send postdata? Why not just array of urls?

Our use case requires passing context associated with the job here. So postdata is abstract here and can be used to pass tokens or any other job context.

edyst-tech · 2019-09-04T09:19:54Z

Thank you so much @hermanzdosilovic for taking your time in reviewing the pull request. We shall respond inline to your comments.

edyst-tech · 2019-09-04T11:17:00Z

This is a good start for #33 but we need to get back on the whiteboard for this to be closed.

I would suggest that you answer few questions:

Why do you need multiple callback URLs?

In our use case, more than one service ought to be notified of the job result/status.

How can we log the success of calling this URL?

How can user see the status of calling this URL?

In our use case, we don't use the DB records at all. If we are not notified through the webhook we treat as failure. But agree, we ought to log failures/status for debugs and for other use cases DB update with webhook status/response might be warranted.

Does the call_webhooks function need to be in the IsolateJob or can it be in some other job so that we don't spend time on sending the data. Maybe call_webhooks can be called by some other non priority worker that just sends data to the workers. After all, we want the code to be executed as fast as possible and we want to free workers as soon as we can so they can run some other code.

Yeah, possible. But we felt this is simpler and faster too as we would be notified immediately on job completion. This would allow us to migrate away from the polling job we already have and avoids the delay in seeing the job result. Having another job would also mean polling or notifications for job status.

What data do we need to send to the callback? Can user decide what it needs?

At this time we are sending the entire submission record and webhook postdata. But yes, in future we can extend this by accepting another key to specify a subset of fields to be included/filtered out in addition to < url, postData >

Think about these questions and try answering them so we can iterate on this feature.

Thanks for your contribution.

Thank you @hermanzdosilovic for sharing this project!

hermanzdosilovic · 2019-09-23T21:32:45Z

What is the status of this PR? Do I need to create another review? Sorry for the late response, hope you are using this feature on your own instance, but I am really interested in having this in production.

hermanzdosilovic · 2020-03-27T01:17:28Z

This feature is now part of v1.6.0.

srinivas33 added 7 commits August 29, 2019 12:32

add new metadata field to submission model

464b82e

permit metadata param

0c03acd

update rails version

b9548c3

rename metadata to webhooks and add webhooks to permitted fields

07f4764

install httparty and call webhooks

a5e7a7b

set json header while sending post request

234d9c4

only send submission data

d75609b

hermanzdosilovic reviewed Sep 1, 2019

View reviewed changes

hermanzdosilovic requested changes Sep 1, 2019

View reviewed changes

hermanzdosilovic added discussion enhancement labels Sep 1, 2019

srinivas33 added 6 commits September 5, 2019 13:51

handle null url case while calling webhooks

12a0b4d

add httparty to gemfile

d4e1696

remove httparty at the end of gemfile

79a4f34

add json and tz-infodata to gemfile

53671b1

handle webhooks null case

7d69f2f

remove unused data hash/dict

06a2606

edyst-tech requested a review from hermanzdosilovic October 9, 2019 10:12

hermanzdosilovic force-pushed the master branch 2 times, most recently from 84dafbd to f886f54 Compare January 2, 2020 00:46

hermanzdosilovic closed this Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a patch to support webhooks #98

add a patch to support webhooks #98

edyst-tech commented Aug 29, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 5, 2019

hermanzdosilovic left a comment

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

hermanzdosilovic Sep 1, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

hermanzdosilovic Sep 1, 2019

edyst-tech Sep 4, 2019

edyst-tech commented Sep 4, 2019

edyst-tech commented Sep 4, 2019

hermanzdosilovic commented Sep 23, 2019

hermanzdosilovic commented Mar 27, 2020

add a patch to support webhooks #98

add a patch to support webhooks #98

Conversation

edyst-tech commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hermanzdosilovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edyst-tech commented Sep 4, 2019

edyst-tech commented Sep 4, 2019

hermanzdosilovic commented Sep 23, 2019

hermanzdosilovic commented Mar 27, 2020