Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
User metadata on workflows/events for user interface usage #371
User metadata on workflows/events for user interface usage #371
Changes from 6 commits
db555cc
2da3c29
d5b9376
3839519
cb07811
3d93adc
c89cf6e
ae77172
1e8c2d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why choose the field number 301? Why not 18 (next immediate available number) or 100 (if we want to group in hundreds)?
Just curious about the convention we use when picking field numbers in proto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason why the person who added
HistoryEvent.worker_may_ignore
as300
, so that it would not interfere with our continually added, incrementally numbered attribute fields below. I chose 301 instead of 300 just to match history event's equivalent, but don't have to. The field number doesn't really matter much here, so it leaves room for attributes to keep adding.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we considered this as a map from string to Payload.. why not do that? (For the record I like this better, just wondering what the reasoning was.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a few of reasons that was rejected (there's an internal document discussing this). The primary reasons were that 1) the stringly-typed contract was too loose, 2) the keys are not user-defined, and 3) it unnecessarily increased history size for fixed keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is safe for reworking in an incompatible way because it's just models for a well-known workflow query. See description to understand the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always returned in query responses and goes through the user's DC, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a Payload in history and a string here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this object is a query result and query results are payloads already (so already subject to encryption in transit) whereas in history if you want something encrypted at rest it needs to be a payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It this okay to break? Do we have any consumers of this as protos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's ok to break, no we don't have any consumers of it except TypeScript SDK internally (and not this field and we only use this as JSON not proto). Explained in description and in comment.