-
Notifications
You must be signed in to change notification settings - Fork 103
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
Refactor runAsUser into tablehelper package #1653
Conversation
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.
Hrm... this might be okay, but it is a bit of a sharp edge.
tablehelpers.Exec enforces that it's called with a timeout
, and ensures consistent logging. I'm not totally sure the signature is correct, but it does unify a lot of things.
In contrast, this takes a cmd
and runs it. Which, in some ways, is a better idiom. But I'm not sure how we can enforce things like timeouts, and create consistent logging.
I'd be inclined to make a functional argument on tablehelpers.Exec
that takes RunAs(uid)
The more arduous path would be to rethink how we handle exec. I can imagine passing cmd around, and maybe there are other patterns.
Curious where y'all land
I don't feel strongly about it either way. I think enforcing the timeout/logging is a plus, but I'm not terribly worried about not enforcing either since we do a good job including that in the first place and/or requesting it in code review. |
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.
Looks pretty good. As Becca said -- drop the WithUid
into a _posix
flavor.
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.
🔥
Cleaning up the runAsUser idiom I was using in a couple of places for the table generates.