Fix logging output for teleport configure commands.#38257
Merged
Conversation
AntonAM
force-pushed
the
anton/configure-fix-logging
branch
from
567c130 to
93e71bd
Compare
marcoandredinis
approved these changes
Feb 15, 2024
rosstimothy
reviewed
Feb 15, 2024
| "github.com/aws/aws-sdk-go-v2/config" | ||
| "github.com/aws/aws-sdk-go-v2/service/iam" | ||
| "github.com/gravitational/trace" | ||
| log "github.com/sirupsen/logrus" |
Contributor
There was a problem hiding this comment.
If we don't alias the import and require using logrus.Debugf explicitly it'd be a bit more obvious that we were using the wrong log package.
Suggested change
| log "github.com/sirupsen/logrus" | |
| "github.com/sirupsen/logrus" |
| ctx := context.Background() | ||
|
|
||
| // Ensure we print output to the user. LogLevel at this point was set to Error. | ||
| log.SetLevel(log.InfoLevel) |
Contributor
There was a problem hiding this comment.
can't we reuse the same logger as teleport and change only if debug is set?
Not sure if users want to see info related logs when running something on cloud shell
Contributor
Author
There was a problem hiding this comment.
Yeah, I thought -d was global, but we actually don't have -d for integration commands. But I changed setting of the level to be flexible, so if the future there will be -d it will not get overwritten.
And I think showing those logs by default is helpful for the user, it tells useful information (policies names).
rosstimothy
reviewed
Feb 16, 2024
| } | ||
|
|
||
| log.Printf("IntegrationRole: IAM Policy %q added to Role %q\n", req.IntegrationRoleTAGPolicy, req.IntegrationRole) | ||
| logrus.Printf("IntegrationRole: IAM Policy %q added to Role %q\n", req.IntegrationRoleTAGPolicy, req.IntegrationRole) |
Contributor
There was a problem hiding this comment.
Suggestion: Use Infof instead of Printf to be consistent with the rest of our logging code. Same suggestion for other commands below.
Suggested change
| logrus.Printf("IntegrationRole: IAM Policy %q added to Role %q\n", req.IntegrationRoleTAGPolicy, req.IntegrationRole) | |
| logrus.Infof("IntegrationRole: IAM Policy %q added to Role %q\n", req.IntegrationRoleTAGPolicy, req.IntegrationRole) |
tigrato
approved these changes
Feb 20, 2024
AntonAM
commented
Feb 21, 2024
| ctx := context.Background() | ||
|
|
||
| // Ensure we print output to the user. LogLevel at this point was set to Error. | ||
| utils.InitLogger(utils.LoggingForDaemon, slog.LevelInfo) |
rosstimothy
approved these changes
Feb 21, 2024
AntonAM
added a commit
that referenced
this pull request
Feb 21, 2024
* Fix logging output for 'teleport configure' commands. * Change imported log package name and don't overwrite debug level. * Use Infof() instead of Printf() * Use InitLogger to reset loglevel * Remove unneeded code.
AntonAM
added a commit
that referenced
this pull request
Feb 21, 2024
* Fix logging output for 'teleport configure' commands. * Change imported log package name and don't overwrite debug level. * Use Infof() instead of Printf() * Use InitLogger to reset loglevel * Remove unneeded code.
AntonAM
added a commit
that referenced
this pull request
Feb 21, 2024
* Fix logging output for 'teleport configure' commands. * Change imported log package name and don't overwrite debug level. * Use Infof() instead of Printf() * Use InitLogger to reset loglevel * Remove unneeded code.
This was referenced Feb 21, 2024
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Feb 21, 2024
* Fix logging output for 'teleport configure' commands. * Change imported log package name and don't overwrite debug level. * Use Infof() instead of Printf() * Use InitLogger to reset loglevel * Remove unneeded code.
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Feb 21, 2024
* Fix logging output for 'teleport configure' commands. * Change imported log package name and don't overwrite debug level. * Use Infof() instead of Printf() * Use InitLogger to reset loglevel * Remove unneeded code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR fixes printing of the log output to the user when they are running most
teleport configure ...commands. Log package used was wrong and also log level needed to be adjusted for those commands.As an alternative fix, we can just turn all
log.Printf()usage in codepath of those commands intofmt.Pritnf(), but it doesn't feel that good to usefmt.Printf()inside/lib/integrations/awsoidccode. On the other hand, log output looks better for user in that case (no INFO label and no filename:linenumber stuff at the end) and we already do it for theexternalauditstoragecommand, which I didn't touch.Changelog: Fix logging output for
teleport configure ...commands.