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

Enhancement: Add environment variable to set the kpm output information level. #586

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rohanraj123
Copy link

@Rohanraj123 Rohanraj123 commented Jan 14, 2025

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

fix #124

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

Description:
This PR improves the logging mechanism in the KPM package manager by replacing the use of os.Stdin for logging with a custom LogWriter implementation. The changes aim to provide better control over log levels (info and debug) and enhance the maintainability and flexibility of the logging system.

Key Changes:
Custom LogWriter:

Introduced a new LogWriter struct that implements the io.Writer interface.
Supports log levels (info and debug), configurable via the environment variable KPM_LOG_LEVEL.
Defaults to info level if no environment variable is set.
Enhanced KpmClient Logging:

Replaced os.Stdin with logger.NewLogWriter() for the logWriter in KpmClient.
Added a WriteLog method in KpmClient to standardize logging throughout the application.
Modified main.go Initialization:

Updated the main.go file to use kpmcli.WriteLog for logging initialization errors instead of reporter.Fatal.
Event Logging:

Added the ability to log diagnostic messages via the KpmEvent struct, which integrates with the updated KpmClient.
Why These Changes Were Made:
The previous logging mechanism lacked flexibility and log-level filtering, leading to noisy logs in production environments.
The new LogWriter provides a clean and extensible logging interface, making it easier to maintain and debug.
Replacing reporter.Fatal() ensures uniform logging and simplifies the codebase by consolidating logging logic into KpmClient.
How It Works:
The log level can be set via the KPM_LOG_LEVEL environment variable (info or debug).
Logs are written to stdout with appropriate prefixes ([INFO] or [DEBUG]).
Critical errors can still be handled by extending WriteLog to support fatal errors if necessary.
Future Improvements:
Add more log levels (e.g., warn, error, fatal) for greater logging granularity.
Consider reintroducing reporter.Fatal() or adding equivalent behavior for handling critical errors.
Testing:
Verified logs are written to stdout with appropriate prefixes and log levels.
Tested info and debug logging in different environments by setting the KPM_LOG_LEVEL variable.
Ensured no runtime errors during client initialization and command execution.
Impact:
Improves log readability and debugging.
Provides a scalable foundation for future enhancements to logging functionality.

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@Rohanraj123 Rohanraj123 changed the title Cleaned the code Enhancement: Add environment variable to set the kpm output information level. Jan 14, 2025
@Peefy
Copy link
Contributor

Peefy commented Jan 14, 2025

Hello @Rohanraj123

Thanks for the contribution. We can remove the empty file in the PR pkg/checker/scripts/registry_auth/htpasswd and fix the unit test CI issue.

@Rohanraj123
Copy link
Author

Hey @Peefy , I have removed the empty file. Can you please review the PR.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12803314730

Details

  • 12 of 26 (46.15%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 41.968%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/logger/logwriter.go 11 13 84.62%
pkg/client/client.go 1 7 14.29%
pkg/event/event.go 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/client/deperated.go 6 40.89%
Totals Coverage Status
Change from base Build 12760666391: -0.06%
Covered Lines: 4172
Relevant Lines: 9941

💛 - Coveralls

@Peefy Peefy requested a review from zong-zhe January 16, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants