Implement "Author's packages", "Added project tags", and "Removed project tags" Herald fields
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	epriestley
	Jan 17 2020, 5:05 PM

Description

See PHI1609. This issue effectively requests an [ Author's packages ] field in Herald, to write this kind of rule for reviewer management in Differential:

[ Affected packages ][ include ][ X ]
[ Author's packages ][ do not include ][ X ]

See PHI1553. See also https://discourse.phabricator-community.org/t/is-there-a-way-to-make-a-herald-rule-only-trigger-based-on-an-action-not-on-a-state/3316/. Previously, see T13283.

These are broadly requests for "Added project tags" and "Removed project tags" fields. I'm satisfied that T13283 is sticking so these are now reasonable to implement.

Revisions and Commits

rP Phabricator
	D20956	rP6d4c6924d6d9 Update Herald rule creation workflow to use more modern UI elements
	D20962	rP46fcd135ae68 (stable) Fix an issue where Herald rules could fail to evaluate at post-commit…
	D20962	rP42e46bbe5a91 Fix an issue where Herald rules could fail to evaluate at post-commit time
	D20957	rPc99485e8a00d Add "Author's Packages" and "Committer's Packages" Herald rules for Commits and…
	D20955	rP6628cd2b4f7b In Herald "Commit" rules, use repository identities to identify authors and…
	D20953	rP41f143f7feeb Respect repository identities when figuring out authors/committers in Herald…
	D20951	rPa0a346be347e In Herald transcripts, render some field values in a more readable way
	D20949	rP19662e33bc45 In Herald transcript rendering, don't store display labels in keys
	D20948	rPa5a9a5e0027f Remove legacy pre-loading of handles from Herald rendering
	D20947	rPb38449ce8f4b Implement an "Author's packages" Herald field for Differential
	D20946	rP6c4500046f7c Add "Project tags added" and "Project tags removed" fields in Herald

Related Objects

Mentioned In: 2020 Week 5 (Very Late January)
Mentioned Here: D20947: Implement an "Author's packages" Herald field for Differential
T13283: When Herald acts, include the transaction group as part of the "state" it acts upon

Event Timeline

epriestley triaged this task as Wishlist priority.Jan 17 2020, 5:05 PM

epriestley created this task.

Herald added a subscriber: amckinley. · View Herald TranscriptJan 17 2020, 5:05 PM

epriestley added a revision: D20946: Add "Project tags added" and "Project tags removed" fields in Herald.Jan 21 2020, 7:31 PM

epriestley added a revision: D20947: Implement an "Author's packages" Herald field for Differential.Jan 23 2020, 2:17 AM

D20947 does not implement "Author's packages" as a "Commit Content" field, nor as a "Commit Content (Hook)" field. The reason for this is that getting the modern authorPHID in both cases is somewhat complicated.

"Commit Content" fields currently rely on authorPHID on CommitData. This isn't completely effective given that author identities now exist.
"Commit Content (Hook)" fields currently rely on an explicit inline identity query, but bypass the actual identity table. So they'll find the "naive" answer, but may get the wrong answer if an identity has been remapped. This should be updated.

Both cases are somewhat buggy for other "Author ..." fields today; the pathways should be sorted out before new fields are added.

epriestley added a commit: rP6c4500046f7c: Add "Project tags added" and "Project tags removed" fields in Herald.Jan 23 2020, 2:21 AM

epriestley added a commit: rPb38449ce8f4b: Implement an "Author's packages" Herald field for Differential.Jan 23 2020, 2:27 AM

Remaining work:

HeraldPreCommitContentAdapter->lookupUser() should use DiffusionRepositoryIdentityEngine to resolve an identity, not DiffusionResolveUserQuery. Currently, the wrong author will be looked up when an identity exists and is explicitly bound to a non-default user.
HeraldPreCommitContentAdapter->lookupUser() should probably cache results (<raw author string -> resolved identity> on the Adapter is likely an appropriate scope).
HeraldCommitAdapter should load identities by default.
- It looks like this should happen in setObject()?
- setObject() has no prototype, but the root class calls it. Yikes.
Existing rules which use author or committer PHIDs should use the identity versions.
Once this is all sorted out, "Author's packages" fields should be implemented for both Commit flavors.