-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Only call 'user.Current' when we really need to #24156
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.
@capnspacehook Before we merge this, let's cut a dev release with this change and share it with the customer. Drone is having some issues atm though, I'll let you know when it's ready. For now I'll add do-not-merge label.
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.
@capnspacehook Can we provide an actionable error message if user.Current()
is slow for users here as well?
This is a good workaround, but it's not clear that the user will ever know if/when to use this workaround.
@russjones replaced all |
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.
In addition to the comments, please add test coverage.
fe31c6b
to
891dec9
Compare
891dec9
to
9d9580d
Compare
Calling 'user.Current' can be extremely slow in some instances, so avoid it when possible.
9d9580d
to
a563375
Compare
@capnspacehook See the table below for backport results.
|
In #24156 we removed a lot of unnecessary calls to user.Current, which can take a long time in Active Directory environments. This commit removes a few other uses of user.Current where we were doing a full user lookup only to find the user's home directory, which is usually more readily available via the environment.
In #24156 we removed a lot of unnecessary calls to user.Current, which can take a long time in Active Directory environments. This commit removes a few other uses of user.Current where we were doing a full user lookup only to find the user's home directory, which is usually more readily available via the environment.
In #24156 we removed a lot of unnecessary calls to user.Current, which can take a long time in Active Directory environments. This commit removes a few other uses of user.Current where we were doing a full user lookup only to find the user's home directory, which is usually more readily available via the environment.
In #24156 we removed a lot of unnecessary calls to user.Current, which can take a long time in Active Directory environments. This commit removes a few other uses of user.Current where we were doing a full user lookup only to find the user's home directory, which is usually more readily available via the environment.
* User os.UserHomeDir where possible In #24156 we removed a lot of unnecessary calls to user.Current, which can take a long time in Active Directory environments. This commit removes a few other uses of user.Current where we were doing a full user lookup only to find the user's home directory, which is usually more readily available via the environment. * Fix tsh profile dir This fixes a bug introduced in #25950
* User os.UserHomeDir where possible In #24156 we removed a lot of unnecessary calls to user.Current, which can take a long time in Active Directory environments. This commit removes a few other uses of user.Current where we were doing a full user lookup only to find the user's home directory, which is usually more readily available via the environment. * Fix tsh profile dir This fixes a bug introduced in #25950
Calling 'user.Current' can be extremely slow in some instances, so avoid it when possible.
Updates https://github.com/gravitational/customer-sensitive-requests/issues/25.