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

584 handle sony ci webhook #597

Merged
merged 1 commit into from
Jul 27, 2021
Merged

584 handle sony ci webhook #597

merged 1 commit into from
Jul 27, 2021

Conversation

afred
Copy link
Contributor

@afred afred commented Jul 20, 2021

No description provided.

@afred afred requested review from jasoncorum and foglabs July 20, 2021 17:38
@afred afred force-pushed the 585-handle-sony-ci-webhook branch from b85c3b7 to 5d77d09 Compare July 20, 2021 17:53
Copy link
Contributor

@foglabs foglabs left a comment

Choose a reason for hiding this comment

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

I think the things I tagged are a copy paste error, but otherwise looks 👍

respond_to do |format|
if @sony_ci_webhook_log.save
format.html { redirect_to @sony_ci_webhook_log, notice: 'Webhook log was successfully created.' }
format.json { render :show, status: :created, location: @sony_ci_webhook_log }
Copy link
Contributor

Choose a reason for hiding this comment

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

should this json output be rendering a template?

respond_to do |format|
if @sony_ci_webhook_log.update(sony_ci_webhook_log_params)
format.html { redirect_to @sony_ci_webhook_log, notice: 'Webhook log was successfully updated.' }
format.json { render :show, status: :ok, location: @sony_ci_webhook_log }
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

@afred afred Jul 21, 2021

Choose a reason for hiding this comment

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

Ack... i meant to get rid of all actions pertaining to WebhookLogs that were not #show or #index. The WebhgookLog records get created by virtue of Sony Ci sending requests to the /webhooks/[action] endpoint(s), so we don't currently have a use case for adding/editing/deleting the WebhookLog records directly. I'll fix this.

fwiw, you're correct, I think those actions responding with format.json will try to render a app/views/sony_ci/webhook_logs/[action].json.jbuilder, which is a ruby file that uses a ruby DSL to build JSON structures from ActiveRecord models. I believe I removed the tests for these controller actions, hence no errors on Travis.

all of this I learned yesterday after having used rails g scaffold ... which dumps a buuunch of files into your code.

@afred afred force-pushed the 585-handle-sony-ci-webhook branch from 5d77d09 to 5f07820 Compare July 21, 2021 17:07
* Adds SonyCi::WebhooksController#save_sony_ci_id which receives a webhook request from Sony Ci
  and finds an Asset based on the filename, and if found, saves the Sony Ci ID to the Asset.
* Logs webhook activity from Sony Ci in the sony_ci_webhook_logs table (model name SonyCi::WebhookLog).
  NOTE: Scaffolding was used to generate WebhookLog code. Any generated code that we will likely use soon
  was kept, the rest was not added to the git repo.

Closes #584.
@afred afred force-pushed the 585-handle-sony-ci-webhook branch from 5f07820 to 90865c0 Compare July 21, 2021 17:13
@afred afred changed the title 585 handle sony ci webhook 584 handle sony ci webhook Jul 23, 2021
@afred
Copy link
Contributor Author

afred commented Jul 23, 2021

NOTE: the branch name beginning in 585 is misleading here, this PR is a solution for #584, not 585.

@afred
Copy link
Contributor Author

afred commented Jul 26, 2021

and finally, in lieu of Travis buids...
image

@afred afred merged commit 92b56f4 into develop Jul 27, 2021
@afred afred deleted the 585-handle-sony-ci-webhook branch July 27, 2021 20:30
orangewolf pushed a commit to notch8/ams that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants