Page MenuHomePhabricator

Handle crash when build plan has mix of valid and invalid build steps
AbandonedPublic

Authored by hach-que on Jul 26 2016, 5:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 4, 7:05 AM
Unknown Object (File)
Feb 16 2024, 12:03 AM
Unknown Object (File)
Feb 7 2024, 7:41 AM
Unknown Object (File)
Feb 5 2024, 7:03 PM
Unknown Object (File)
Feb 5 2024, 4:24 PM
Unknown Object (File)
Feb 3 2024, 11:30 AM
Unknown Object (File)
Feb 3 2024, 11:29 AM
Unknown Object (File)
Feb 3 2024, 11:29 AM
Subscribers
Restricted Owners Package

Details

Summary

Resolves T11373. Currently Harbormaster gracefully handles a scenario where the build step implenementation is missing for a step. However, dependency calculations on valid build steps may cause them to attempt to access the implementations of other invalid build steps (in particular, when attempting to validate whether their input artifacts are available from other build steps).

When this happens, you get a blunt page crash like this:

pasted_file (244×807 px, 19 KB)

Because of this page crash, there's no way to fix the invalid configuration from the web UI, and the user has to go and remove things from the database manually.

Instead, after this change, the page doesn't crash, but shows "Unable to calculate dependencies for this build step" when this issue occurs.

Test Plan

Had a page with a mix of valid and invalid build steps, where the valid build step had an input artifact. Observed the crash. Applied this patch and saw the crash go away.

Diff Detail

Repository
rP Phabricator
Branch
hmprotect (branched from deploy)
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 13127
Build 16794: Run Core Tests
Build 16793: arc lint + arc unit

Event Timeline

hach-que retitled this revision from to Handle crash when build plan has mix of valid and invalid build steps.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
Owners added a subscriber: Restricted Owners Package.Jul 26 2016, 5:25 AM
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
147

Moving this prevents a double up on the build step when the error is being rendered.

hach-que edited edge metadata.

Not sold on this approach although I have had this exact problem a few times and this papers over the ugly about as well as you can I think?

src/applications/harbormaster/controller/HarbormasterPlanViewController.php
134

PhutilMissingSymbolException is probably scoped better for this specific problem.

135

Should read "if another step", rather than "if another plan"

epriestley edited edge metadata.

I'd like to be a little more surgical about this -- we can know if other build steps are valid when we reach this logic, and don't need to ask the user to check, right? Something like:

  • Check if all steps are valid first.
  • If any aren't, don't try computing dependencies. Instead, show "Unable to calculate dependencies because steps are invalid." without a question.

Then we also won't swallow exceptions arising from other problems.

This revision now requires changes to proceed.Jul 26 2016, 2:13 PM
hach-que edited edge metadata.
  • Be more surgical
Owners added a subscriber: Restricted Owners Package.Jul 28 2016, 4:31 AM
This revision is now accepted and ready to land.Jul 28 2016, 2:27 PM

Is this going to break again when he tries to land it?

iiam

If it does, I guess I'll have to fix the issue.

@yelirekim She, not he.

I'll just remove the owners package again before I land it.

Will most likely be landing this Tuesday AEST unless someone else wants to land it for me.