Skip to content
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

Convert lib/sshutils to use slog #49829

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Convert lib/sshutils to use slog #49829

merged 1 commit into from
Dec 10, 2024

Conversation

rosstimothy
Copy link
Contributor

In addition to the conversion, the reverstunnel server now has a slog.Logger provided so that it can be passed into the sshutils.Server, is not used otherwise. A similar migration will be performed in the future for the reversetunnel package.

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Dec 5, 2024
@rosstimothy rosstimothy force-pushed the tross/forward_slog branch 2 times, most recently from aa3bb4b to 6bdf674 Compare December 6, 2024 18:58
Base automatically changed from tross/forward_slog to master December 6, 2024 19:36
@rosstimothy rosstimothy marked this pull request as ready for review December 6, 2024 19:52
Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should some of those context.Background() be context.TODO() instead?

lib/reversetunnel/srv.go Outdated Show resolved Hide resolved
}
c.Log = logger.WithFields(log.Fields{
teleport.ComponentKey: "SCP",
teleport.ComponentFields: log.Fields{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep this in a group? TBH I've never paid attention to this in our use of logrus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A group might be fitting, though what would the appropriate key be? scp?

In addition to the conversion, the reverstunnel server now has
a slog.Logger provided so that it can be passed into the
sshutils.Server, is not used otherwise. A similar migration will
be performed in the future for the reversetunnel package.
@rosstimothy
Copy link
Contributor Author

Should some of those context.Background() be context.TODO() instead?

From RFD 154:

If no context is readily available for the logger to consume, prefer to use a context.TODO if it is possible to get a context to the call site in the future but refactoring is to do be done in a separate unit of work. If there is likely to never be a context provided to the function producing the log output, then prefer to use a context.Background to indicate as such.

I suppose it may be possible to get a context into some of the places, but for others it would likely require a fair bit of refactoring.

@rosstimothy rosstimothy added this pull request to the merge queue Dec 10, 2024
Merged via the queue into master with commit c80bdf8 Dec 10, 2024
40 checks passed
@rosstimothy rosstimothy deleted the tross/sshutils_slog branch December 10, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants