Fix T9662.
Record who initiated the build, and allow this information as a parameter.
In this implementation, a 're-run' keeps the original initiator, which we maybe not desired?
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T9662: Add initiatorPHID build variable to hint at who/what caused the build
- Commits
- Restricted Diffusion Commit
rP1898864b6c45: add initiator.phid parameter to HM builds
Make a HTTP step with initiator.phid, trigger manually, via HM, via ./bin/harbormaster build.
Look at requests made.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
In this implementation, a 're-run' keeps the original initiator, which we maybe not desired?
Seems OK to me for now, although I think it's reasonable to consider making the user who clicked "restart" the initiator in the future if there's a reason for it. None of the eventual results (stay like this, use user who clicked button, keep both original and button clicker) seem particularly more desirable to me offhand.
| src/applications/harbormaster/engine/HarbormasterTargetEngine.php | ||
|---|---|---|
| 166–169 | What is $viewer when not a PhabricatorUser? This seems really suspicious, I think you can drop the instanceof check. | |
| 173 | e.g., this will fatal anyway? | |
| src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php | ||
| 101–106 | Consider using the Harbormaster application PHID here. | |
| src/applications/harbormaster/storage/HarbormasterBuildable.php | ||
| 166 | Probably not too important. | |
| src/applications/harbormaster/storage/build/HarbormasterBuild.php | ||
| 297–298 | Maybe be slightly more vague/inclusive because other things (releases, evil spirits) are likely to be able to initiate builds in the future. | |
| src/applications/harbormaster/engine/HarbormasterTargetEngine.php | ||
|---|---|---|
| 166–169 | Yes, I probably got this from another version. I'll make the setter explicitly check for this. | |
| src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php | ||
| 101–106 | err... why? | |
| src/applications/harbormaster/storage/build/HarbormasterBuild.php | ||
| 297–298 | "user or object"? | |
| src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php | ||
|---|---|---|
| 101–107 | This stuff tends to end up rendered in the UI in some form sooner or later, and we generally can't reasonably special case it. Using the application PHID let us render, say, this, in a transaction log:
...instead of this:
This is consistent with other actions ("Herald added subscribers...", lease owners from bin/drydock, etc). | |
| src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php | ||
|---|---|---|
| 101–107 | I'm not 100% sold about it from the correctness POV, but it makes sense for consistency. | |