Shopify / Shopify/shipit-engine

Stack page links deploys/rollbacks to tasks#show, which 500s on @task.definition.id (nil for deploys)

Open
#1,488 1 comment 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
1.5k
Forks
154
Avg merge
2h 26m
Merged PRs (30d)
3

Description

Summary

On a stack's show page, every entry in the Previous Deploys list (including Shipit::Deploy and Shipit::Rollback records) is linked to tasks#show (/stacks/:id/tasks/:task_id). Clicking on a "deploy" or "rollback" task that way raises:

ActionView::Template::Error (undefined method 'id' for nil)
    app/views/shipit/tasks/show.html.erb:2

so clicking any deploy in Previous Deploys returns a 500.

Note that overriding the link and visiting the same record at /stacks/:id/deploys/:id works fine.

Steps to reproduce

  1. Open a stack that has previous deploys.
  2. Click a deploy (or rollback) in the Previous Deploys list — it links to /stacks/:id/tasks/:task_id.
  3. 500: undefined method 'id' for nil at app/views/shipit/tasks/show.html.erb:2.

Root cause

tasks/show.html.erb renders the task title with @task.definition.id:

<%= @task.active? ? "executing" : "executed" %> <%= @task.definition.id %>

definition is a serialized column populated only for generic shipit.yml tasks.
Deploys and rollbacks don't seem to ever have one (Stack#build_deploy doesn't set it).

This view is only meant for generic tasks — deploys/rollbacks have their own deploys/show and deploys/_deploy. The reason they now reach tasks#show is the deploy list in stacks/show.html.erb:

<%= render partial: 'shipit/tasks/task', collection: @tasks %>

This forces every record through shipit/tasks/_task, which links via stack_task_pathtasks#show. Previously the list used polymorphic rendering:

<%= render @tasks %>

which dispatched each record via to_partial_path: Shipit::Deployshipit/deploys/_deploy and Shipit::Rollback (to_partial_path overridden to 'deploys/deploy') → the same deploy partial, both linking to deploys#show. Only generic tasks rendered tasks/_task.

The change came in 793e1d5cc22fece0eb644160026042a483dc1f49, which replaced render @tasks with the explicit single-partial form.
That seemed to have suppressed the implicit-render warning but loses the per-type partial dispatch for the STI subclasses.

Still present on main / 0.45.1.

Possible fixes

  • Restore polymorphic rendering (render @tasks) for the deploy list in stacks/show.html.erb; or
  • Keep explicit rendering but dispatch per type (deploys/_deploy for Deploy/Rollback, tasks/_task for generic tasks); or
  • Make tasks/show.html.erb tolerate a nil definition (e.g. fall back to @task.title) so a deploy routed there doesn't crash.

Environment

  • shipit-engine 0.45.1 (also reproducible on 0.44.2)
  • Rails 8.1.3

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with app/views/stacks/show.html.erb and compare its rendering of @tasks with the type-specific partials app/views/shipit/tasks/_task and app/views/shipit/deploys/_deploy. Read app/views/shipit/tasks/show.html.erb and reproduce the Previous Deploys navigation; done means deploys and rollbacks no longer reach the generic task view or produce a 500, while generic tasks still work.

Written by the indexing model from the issue text.

Assessment

Tech stack
rails, ruby
Domain
backend, devops
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.