1. 02 Nov, 2015 4 commits
  2. 01 Nov, 2015 1 commit
    • Chad Little's avatar
      Redesign Diviner · c45ba304
      Chad Little authored
      Summary:
      This implements `PHUIDocumentViewPro` which should move to be the base for all documents (Phame, Phriction, Legalpad, Diviner). Overall this feels really good to me, but I'd like to roll it out into Diviner specifically first to work through the issues and then move into other apps and drop `PHUIDocumentView` once everything is converted. Some features are:
      
       - White Background, no border on page
       - Table of Contents is move to hidden menu (more space for documentation)
       - Property List sits under the document
      
      Some design decisions above are in anticipation of Phriction v3 and Unbeta Phame, specifically commenting and maybe some cool new Remarkup text layout options for Phame.
      
      Test Plan:
      Went through tons of pages on Diviner on Desktop, Tablet, Mobile. Bounce back to Phriction to make sure DocumentView CSS changes actually look better there.
      
      {F930518}
      
      {F930519}
      
      {F930520}
      
      {F930521}
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: tycho.tatitscheff, joshuaspence, Korvin
      
      Differential Revision: https://secure.phabricator.com/D14374
      c45ba304
  3. 31 Oct, 2015 4 commits
  4. 30 Oct, 2015 5 commits
    • epriestley's avatar
      Add a setup warning for major clock skew issues · 4d13b6c6
      epriestley authored
      Summary: See IRC. A user had a database set to 8 hours ahead of their web host. Try to catch and warn about these issues.
      
      Test Plan: Artificially adjusted skew, saw setup warning.
      
      Reviewers: avivey, chad
      
      Reviewed By: chad
      
      Differential Revision: https://secure.phabricator.com/D14371
      4d13b6c6
    • epriestley's avatar
      Fix an issue with incorrect authorization handling in Working Copy build steps · f48a8337
      epriestley authored
      Summary:
      Fixes T9669. Two issues:
      
        - We were using `repositoryPHIDs` instead of `blueprintPHIDs` for the list of allowed blueprints. Use the correct value.
        - We weren't enforcing `allowedBlueprintPHIDs` fully correctly. We //did// require an authorization, so the net effect was correct in nearly all cases, but we could have selected from too large a pool in the case where the application itself was doing the authorization (e.g., from the command line).
      
      Test Plan: Ran a build through Drydock/Harbormaster locally.
      
      Reviewers: chad, tycho.tatitscheff
      
      Reviewed By: chad, tycho.tatitscheff
      
      Subscribers: tycho.tatitscheff
      
      Maniphest Tasks: T9669
      
      Differential Revision: https://secure.phabricator.com/D14368
      f48a8337
    • epriestley's avatar
      Allow any {icon} to spin · 096117aa
      epriestley authored
      Summary: We are greedily hoarding this for ourselves, when we could enrich the world.
      
      Test Plan: Used `{icon cog spin}`.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Differential Revision: https://secure.phabricator.com/D14369
      096117aa
    • epriestley's avatar
      Move "Next Step" to a custom field in Differential · 2c3dbc48
      epriestley authored
      Summary:
      Fixes T9672. This was never turned into a custom field, for no particular reason. Convert it into one.
      
      This is substantially similar to the existing "Apply Patch" field, which does the same thing (only shows a command).
      
      We might rethink or remove this eventually (e.g., in a post-"Land Revision" world) but this makes it easier, at the very least.
      
      Test Plan:
        - Viewed a non-accepted revision (no hint).
        - Viewed an accepted revision from a raw diff source (no hint).
        - Viewed an accepted revision from Git (`arc land` hint).
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9672
      
      Differential Revision: https://secure.phabricator.com/D14367
      2c3dbc48
    • epriestley's avatar
      Correct the handle URI for build steps · 1b833787
      epriestley authored
      Summary: Fixes T9674. This was wrong to start with (URI is `/edit/X/`, not `/X/edit/`) but we have a new view page anyway.
      
      Test Plan:
        - Visited an exmaple URI in my browser.
        - Followed a build step link from "Authorized By: ..." in Drydock.
      
      Reviewers: joshuaspence, chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9674
      
      Differential Revision: https://secure.phabricator.com/D14366
      1b833787
  5. 28 Oct, 2015 2 commits
  6. 27 Oct, 2015 5 commits
  7. 26 Oct, 2015 11 commits
    • epriestley's avatar
      Don't show error operations after a successful land operation · cea633f6
      epriestley authored
      Summary:
      Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.
      
      Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).
      
      Also make the table a little nicer, particularly for merge failure output.
      
      Test Plan: {F910385}
      
      Reviewers: chad, Mnkras
      
      Reviewed By: Mnkras
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14348
      cea633f6
    • Michael Krasnow's avatar
      Set a property so that unit tests run on PHP7 · 6e7ceb99
      Michael Krasnow authored
      Summary: Without this change PHP throws because idx() is passed null as the property is not intialzied
      
      Test Plan: arc unit --everything
      
      Reviewers: #blessed_reviewers, epriestley
      
      Reviewed By: #blessed_reviewers, epriestley
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D14345
      6e7ceb99
    • epriestley's avatar
      Provide a username and email when running `git merge --squash` · bbf4ce79
      epriestley authored
      Summary:
      Ref T182. This command should never actually generate a commit because `--squash` prevents that, but `git` seems to sometimes hit a check for username/email configuration (maybe when merging a non-fastforward?).
      
      Give it some dummy values to placate it. This command shouldn't commit anything so these values should never actually be used.
      
      Test Plan: Landed rGITTESTd8c8643cb02bbe60048c6c206afc2940c760a77e.
      
      Reviewers: chad, Mnkras
      
      Reviewed By: Mnkras
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14347
      bbf4ce79
    • epriestley's avatar
      Don't use --ff-only inside "Land Revision" · 5a35dd23
      epriestley authored
      Summary:
      Ref T182. I lifted this logic out of `arc`, but the context is a little different there, and this option is too strict in "Land Revision".
      
      Specifically, it prevents `git` from merging unless the merge is //strictly// a fast-foward, even with `--squash`. That means revisions can't merge unless they're rebased on the current `master`, even if they have no conflicts.
      
      (This whole process will probably need additional refinement, but the behavior without this flag is more reasonable overall than the behavior with it for now.)
      
      Test Plan: Will land stuff in production~~
      
      Reviewers: chad, Mnkras
      
      Reviewed By: Mnkras
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14346
      5a35dd23
    • epriestley's avatar
      Make "Land Revision" show merge conflicts more clearly · 0b24a6e2
      epriestley authored
      Summary:
      Ref T182. We just show "an error happened" right now. Improve this behavior.
      
      This error handling chain is a bit ad-hoc for now but we can formalize it as we hit other cases.
      
      Test Plan:
      {F910247}
      
      {F910248}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14343
      0b24a6e2
    • epriestley's avatar
      Show lease on Repository Operation detail view and awaken on failures · 2326d5f8
      epriestley authored
      Summary:
      Ref T182. Couple of minor improvements here:
      
        - Show the Drydock lease when viewing a Repository Operation detail screen. This just makes it easier to jump around between relevant objects.
        - When tasks are waiting for a lease, awaken them when it breaks or is released, not just when it is acquired. This makes the queue move forward faster when errors occur.
      
      Test Plan:
        - Viewed a repository operation and saw a link to the lease.
        - Did a bad land (intentional merge problem) and got an error in about ~3 seconds instead of ~17.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14341
      2326d5f8
    • epriestley's avatar
      Show the oldest non-failing revision land operation, or the newest failure · a0fba642
      epriestley authored
      Summary:
      Ref T182.
      
        - We just show the oldest operation right now, but we usually care about the oldest non-failure.
        - Only query for actual land operations when rendering the revision operations dialog (maybe eventually we'll show more stuff?).
        - For now, prevent multiple lands / repeated lands or queueing up lands while other lands are happening.
      
      Test Plan: Landed a revision. Tried to land it more / again.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14338
      a0fba642
    • epriestley's avatar
      Make WorkingCopyBlueprint responsible for performing merges · 9c394937
      epriestley authored
      Summary:
      Ref T182. Currently, the "RepositoryLand" operation is responsible for performing merges when landing a revision.
      
      However, we'd like to be able to perform these merges in a larger set of cases in the future. For example:
      
        - After Releeph is revamped, when someone says "I want to merge bug fix X into stable branch Y", it would probably be nice to make that a Buildable and let tests run against it without requring that it actually be pushed anywhere.
        - Same deal if we want a merge-from-Diffusion or cherry-pick-from-Diffusion operation.
        - Similar deal if we want a "random web UI edits from Diffusion".
      
      Move the merging part into WorkingCopy so more applications can share/use it in the future.
      
      A big chunk of this is me making stuff up for now (the ol' undocumented dictionary full of arbitrary magic keys), but I anticipate formalizing it as we move along.
      
      Test Plan: Pushed rGITTEST0d58eef3ce0fa5a10732d2efefc56aec126bc219 up from my local install via "Land Revision".
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T182
      
      Differential Revision: https://secure.phabricator.com/D14337
      9c394937
    • epriestley's avatar
      Remove Drydock host resource limits and give working copies simple limits · c059149e
      epriestley authored
      Summary:
      Ref T9252. Right now, we have very strict limits on Drydock: one lease per host, and one working copy per working copy blueprint.
      
      These are silly and getting in the way of using "Land Revision" more widely, since we need at least one working copy for each landable repository.
      
      For now, just remove the host limit and put a simple limit on working copies. This might need to be fancier some day (e.g., limit working copies per-host) but it is generally reasonable for the use cases of today.
      
      Also add a `--background` flag to make testing a little easier.
      
      (Limits are also less important nowadays than they were in the past, because pools expand slowly now and we seem to have stamped out all the "runaway train" bugs where allocators go crazy and allocate a million things.)
      
      Test Plan:
        - With a limit of 5, ran 10 concurrent builds and saw them finish after allocating 5 total resources.
        - Removed limit, raised taskmaster concurrency to 128, ran thousands of builds in blocks of 128 or 256.
          - Saw Drydock gradually expand the pool, allocating a few more working copies at first and a lot of working copies later.
          - Got ~256 builds in ~140 seconds, which isn't a breakneck pace or anything but isn't too bad.
          - This stuff seems to be mostly bottlenecked on `sbuild` throttling inbound SSH connections. I haven't tweaked it.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9252
      
      Differential Revision: https://secure.phabricator.com/D14334
      c059149e
    • epriestley's avatar
      Give Harbormaster build steps a "View" page · 3f193cb9
      epriestley authored
      Summary:
      Fixes T9519. Right now, build steps go straight from the build to the edit screen.
      
      This means that there's no way to see their edit history or review details without edit permission. In particular, this makes it a bit harder to catch the Drydock Blueprint authorization warnings from T9519.
      
        - Add a standard view screen.
        - Add a little warning callout to blueprint authorizations.
      
      This also does a bit of a touchup on the weird dropshadow element from T9586. Maybe not totally design-approved now but it's less ugly, at least.
      
      Test Plan:
      {F906695}
      
      {F906696}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9519
      
      Differential Revision: https://secure.phabricator.com/D14330
      3f193cb9
    • epriestley's avatar
      Give Harbormaster Build Plans real policies · 5ee4a1a3
      epriestley authored
      Summary:
      Ref T9614. Currently, a lot of Build Plan behavior is covered by a global "can manage" policy.
      
      One install in particular is experiencing difficulty with warring factions within engineering aborting one another's builds.
      
      As a first step to remedy this, and also generally make Harbormaster more flexible and bring it in line with other applications in terms of policy power:
      
        - Give Build Plans normal view/edit policies.
        - Require "Can Edit" to run a plan manually.
      
      Having "Can View" on plans may be a little weird in some cases (the status of a Buildable might be bad because of a build you can't see) but we can cross that bridge when we come to it.
      
      Next change here will require "Can Edit" to abort a build. This will reasonably allow installs to reserve pause/abort for administrators/adults. (I might let anyone restart a plan, though?)
      
      Test Plan:
        - Created a new build plan.
        - Verified defaults were inherited from application defaults (swapped them around, too).
        - Saved build plan.
        - Edited policies.
        - Verified autoplans get the right policies.
        - Verified old plans got migrated properly.
        - Tried to run a plan I couldn't edit (denied).
        - Ran a plan from CLI with `bin/harbormaster`.
        - Tried to create a plan with an unprivileged user.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9614
      
      Differential Revision: https://secure.phabricator.com/D14321
      5ee4a1a3
  8. 25 Oct, 2015 4 commits
    • epriestley's avatar
      Make WorkingCopy build step slightly more durable · 43569d4e
      epriestley authored
      Summary: Fixes T9631. Build steps created before I added this option may not have it specified, which could throw later. Make handling a little more robust.
      
      Test Plan: Will ask @yelirekim to report back.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T9631
      
      Differential Revision: https://secure.phabricator.com/D14336
      43569d4e
    • epriestley's avatar
      Write more detailed documentation about Differential inlines · 2beeb2fa
      epriestley authored
      Summary: Ref T9628. The porting feature has been fairly stable for a while, so make some reasonable effort to document how it works and some of the tradeoffs it involves.
      
      Test Plan: Generated and read documentation.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9628
      
      Differential Revision: https://secure.phabricator.com/D14335
      2beeb2fa
    • epriestley's avatar
      Prevent mailing lists from being bin/auth recover'd · 59c93171
      epriestley authored
      Summary:
      Fixes T9610.
      
        - We currently permit you to `bin/auth recover` users who can not establish web sessions (but this will never work). Prevent this.
        - We don't emit a tailored error if you follow one of these links. Tailor the error.
      
      Even with the first fix, you can still hit the second case by doing something like:
      
        - Recover a normal user.
        - Make them a mailing list in the DB.
        - Follow the recovery link.
      
      The original issue here was an install that did a large migration and set all users to be mailing lists. Normal installs should never encounter this, but it's not wholly unreasonable to have daemons or mailing lists with the administrator flag.
      
      Test Plan:
        - Tried to follow a recovery link for a mailing list.
        - Tried to generate a recovery link for a mailing list.
        - Generated and followed a recovery link for a normal administrator.
      
      {F906342}
      
      ```
      epriestley@orbital ~/dev/phabricator $ ./bin/auth recover tortise-list
      Usage Exception: This account ("tortise-list") can not establish web sessions, so it is not possible to generate a functional recovery link. Special accounts like daemons and mailing lists can not log in via the web UI.
      ```
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9610
      
      Differential Revision: https://secure.phabricator.com/D14325
      59c93171
    • Yongmin Hong's avatar
      Provide an application link for Ponder Answer PHID type · a39ec26a
      Yongmin Hong authored
      Summary: Ref T9625. I want this to be fixed ASAP hence here's the patch.
      
      Test Plan:
       - ~~Apply D14323~~ (This patch was made before it was merged)
       - Apply this patch
       - voila! Now I see the Ponder answer has correct logo.
      
      {F906357}
      
      Reviewers: #blessed_reviewers, epriestley
      
      Reviewed By: #blessed_reviewers, epriestley
      
      Subscribers: Korvin, revi
      
      Maniphest Tasks: T9625
      
      Differential Revision: https://secure.phabricator.com/D14331
      a39ec26a
  9. 24 Oct, 2015 4 commits