sync lex: #account firehose event; account status errors; and getAccountStatus endpoint #2263
Conversation
| "description": "Indicates that the account has a repository which can be fetched from the host that emitted this event." | ||
| }, | ||
| "status": { | ||
| "type": "string", |
There was a problem hiding this comment.
made a tweak here
with token values we normally just use a string type & list the token values in knownValues.
see com.atproto.moderation.defs for an example with report types
There was a problem hiding this comment.
To be honest I am not even sure if we need tokens here— its possible it could be non-namespaced strings.
In my mind the value in tokens is that it allows extensibility by other namespaces, especially in cases where it's friendly for applications to be able to use values that will never make it into the lexicon's knownValues. I don't think that's necessarily needed or even desirable here in the sync endpoints. One reason we may not want that, is that it's nice to keep the values one-to-one with the error types on these sync endpoints, which are not namespaced. That allows you to understand the repo's status fully through the response of e.g. sync.getRepo.
There was a problem hiding this comment.
That's a good point - and I do tend to agree that this isn't a place that we're asking for extensibility on. Any changes should be part of the formal sync protocol 🤔
So maybe we just do hardcoded values?
There was a problem hiding this comment.
That would be my vote! I still support keeping it an open union as a little safety net.
There was a problem hiding this comment.
it feels like a place "hardcoded" would make sense. agree on keeping it open-union semantics.
|
Actually on second pass i think i like |
|
Also - we should codegen in this PR before merging 👍 |
lexicons/com/atproto/sync/defs.json
Outdated
| }, | ||
| "deactivated": { | ||
| "type": "token", | ||
| "description": "Repo hosting status indicating that the repository has been pause and should not be re-distributed, usually on request of the account holder. This may be temporary or indefinite." |
There was a problem hiding this comment.
s/pause/paused/ (though I'm not sure if "repository has been paused" is the best word here...)
| "properties": { | ||
| "did": { "type": "string", "format": "did" }, | ||
| "active": { "type": "boolean" }, | ||
| "status": { | ||
| "type": "string", | ||
| "description": "If active=false, this optional field indicates a possible reason for why the account is not active. If active=false and no status is supplied, then the host makes no claim for why the repository is no longer being hosted.", | ||
| "knownValues": ["takendown", "suspended", "deactivated"] | ||
| }, | ||
| "rev": { | ||
| "type": "string", | ||
| "description": "Optional field, the current rev of the repo, if active=true" | ||
| } | ||
| } |
There was a problem hiding this comment.
Should results in sync.listRepos also include the hosting status, and perhaps generally have the same shape as this?
| "active": { | ||
| "type": "boolean", | ||
| "description": "Indicates that the account has a repository which can be fetched from the host that emitted this event." | ||
| }, | ||
| "status": { |
There was a problem hiding this comment.
I'm alright with keeping active around, though I am not sure it is necessary. It allows a PDS to express some conflicting information, like setting a status with active=true.
There was a problem hiding this comment.
just to expand on the reasoning there a bit, I wanted to future-proof a bit against us adding additional status strings in the future. If simple infra implementations match on the status string to either make the account visible or not make the account visible (who knows what kind of weird match/switch control flow they might have), then adding a status code could result in the incorrect behavior. Making it a boolean flag makes the "is account visible or not" behavior very clear, with the status being more of an end-user-visible context and less of a high-stakes "is content redistributed at all".
still agree with your point, just wanted to document that bit of thinking
As discussed earlier today. Did not include any changes to the
#identityevent as we don't have consensus on that yet.Notes/details:
getLatestCommit. I wedgedrevin togetAccountStatusand we could put thecidin there too, and just deprecategetLatestCommit. In that case maybegetRepoStatusis a better name, and clearer in the case of Relay (which doesn't really have an "account", just the repo)