-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implement "owners" in workers #621
Conversation
a806e34
to
701459e
Compare
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 seems fine in principle, but I don't think the implementation works as written.
701459e
to
70deeec
Compare
d4909a3
to
baf59b7
Compare
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.
Any specific reason we're only doing this for dedicated workers? I think the same approach also works for shared workers.
763c992
to
ec2da0e
Compare
@jgraham So I'm a little conflicted right now about exposing all owners. What I really want to expose is the "initial owner", not necessarily all of them. If we want to expose all of them, then we'd need events like What do you think about extending this to that length? |
I think dedicated workers only ever have one owner, so for that case the events don't seem necessary. For shared workers it makes more sense to me to expose the owners in the same way we do for dedicated workers, but not add lifecycle events until there's clear user demand that it does to not expose the owners because we don't also have lifecycle events. So, for now, I'd just add the field and information to all worker types, so that we at least have a consistent basis to build from, and consider extending the support later if necessary. |
ec2da0e
to
f5f87f8
Compare
Done. |
b57783e
to
7d7e876
Compare
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 basically looks fine, but I think there's a technical error copied from elsewhere in the spec.
7d7e876
to
af0ceaf
Compare
SHA: bb3f568 Reason: push, by jrandolf Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Every dedicated worker has an outside setting that it's created under. This info is needed to understand the tree structure of dedicated workers.
Preview | Diff