Skip to content

Conversation

@Shubhamag12
Copy link
Contributor

We want to migrate from the external github.com/sirupsen/logrus dependency to Go's standard library log/slog package. This will eliminate external dependencies and improve performance.

Fixes: #4930

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 28, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Shubhamag12 / name: Shubham Agarwal (5b9ff22)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 28, 2025
@k8s-ci-robot k8s-ci-robot requested a review from Kavinjsir July 28, 2025 11:30
@k8s-ci-robot
Copy link
Contributor

Welcome @Shubhamag12!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 28, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @Shubhamag12. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 28, 2025
@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 29, 2025
@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 29, 2025
@camilamacedo86 camilamacedo86 requested a review from Copilot August 3, 2025 19:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates from the external github.com/sirupsen/logrus logging library to Go's standard library log/slog package. This change eliminates external dependencies and modernizes the logging approach to use structured logging throughout the codebase.

  • Replaces all logrus imports with log/slog imports across the entire codebase
  • Converts log method calls from logrus format (e.g., log.Printf, log.Errorf) to slog structured logging (e.g., log.Info, log.Error with key-value pairs)
  • Introduces a custom logging handler in pkg/logging/handler.go with colored output and structured attribute formatting

Reviewed Changes

Copilot reviewed 49 out of 50 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/logging/handler.go New custom slog handler implementation with colored output and structured logging
cmd/cmd.go Updated to use slog and initialize custom logging handler
go.mod Removed logrus dependency
Multiple scaffolding files Converted logrus calls to slog structured logging with key-value pairs
Test and utility files Updated logging imports and method calls to use slog
Documentation Updated example code to reference slog instead of logrus

f.PackageName = "controller"

log.Println("creating import for %", f.Resource.Path)
log.Info("creating import%", "resource_path", f.Resource.Path)
Copy link

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

The log message contains a malformed format string with '%' at the end. It should be 'creating import for' to be grammatically correct.

Suggested change
log.Info("creating import%", "resource_path", f.Resource.Path)
log.Info("creating import for", "resource_path", f.Resource.Path)

Copilot uses AI. Check for mistakes.
if err != nil {
if errors.Is(err, afero.ErrFileNotFound) {
log.Warnf("Unable to find %s : %s .\n"+
log.Warn("Unable to find boilerplate file"+
Copy link

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

Missing space between 'file' and the concatenated string. Should be 'Unable to find boilerplate file. ' with a period and space.

Suggested change
log.Warn("Unable to find boilerplate file"+
log.Warn("Unable to find boilerplate file. "+

Copilot uses AI. Check for mistakes.
if err != nil {
log.Warningf("Unable to find the certificate name replacement for CRD %s "+
"to uncomment in %s. Conversion webhooks require this replacement to inject "+
log.Warn("Unable to find the certificate name replacement for CRD"+
Copy link

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

Missing space between 'CRD' and 'to uncomment'. Should be 'Unable to find the certificate name replacement for CRD ' with a space.

Suggested change
log.Warn("Unable to find the certificate name replacement for CRD"+
log.Warn("Unable to find the certificate name replacement for CRD "+

Copilot uses AI. Check for mistakes.
log.Warningf("Unable to find the target configurations with kustomizeconfig.yaml"+
"to uncomment in the file %s. ConverstionWebhooks requires this configuration "+
"to be uncommented to inject CA", kustomizeCRDFilePath)
log.Warn("Unable to find the target configurations with kustomizeconfig.yaml"+
Copy link

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

Missing space between 'kustomizeconfig.yaml' and 'to uncomment'. Should be 'Unable to find the target configurations with kustomizeconfig.yaml ' with a space.

Suggested change
log.Warn("Unable to find the target configurations with kustomizeconfig.yaml"+
log.Warn("Unable to find the target configurations with kustomizeconfig.yaml "+

Copilot uses AI. Check for mistakes.
@camilamacedo86
Copy link
Member

Hi @Shubhamag12

That is great 🎉 I tested locally.
The Copilot AI found some small nits
Could you please address those so that we can get this one merged?

@Shubhamag12 Shubhamag12 force-pushed the issue-4930-logrus branch 2 times, most recently from 4491057 to 90ad1ea Compare August 4, 2025 05:52
@camilamacedo86
Copy link
Member

/test pull-kubebuilder-test

@camilamacedo86
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Shubhamag12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2025
@k8s-ci-robot k8s-ci-robot merged commit 3c015e0 into kubernetes-sigs:master Aug 4, 2025
18 checks passed
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Aug 24, 2025
Follow up: kubernetes-sigs#4968

Testing alpha update and generate revealed logs were not in the right format.

Changes:
- Initialize consistent logging handler in alpha commands to fix level=info msg= format
- Replace log.Fatalf with slog.Error + os.Exit(1) for proper error handling
- Standardize log imports to use log "log/slog" across all files
- Remove redundant command info from RunCmd logging output
- Ensure all kubebuilder processes use the same colored INFO/WARN format

Before: level=warning msg="Using current working directory..."
After:  WARN Using current working directory...

Assisted-by: Cursor
k8s-ci-robot added a commit that referenced this pull request Aug 24, 2025
🌱  (fix): Fix inconsistent logging format  ( Follow up: #4968 )
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Aug 25, 2025
Follow up: kubernetes-sigs#4968

Testing alpha update and generate revealed logs were not in the right format.

Changes:
- Initialize consistent logging handler in alpha commands to fix level=info msg= format
- Replace log.Fatalf with slog.Error + os.Exit(1) for proper error handling
- Standardize log imports to use log "log/slog" across all files
- Remove redundant command info from RunCmd logging output
- Ensure all kubebuilder processes use the same colored INFO/WARN format

Before: level=warning msg="Using current working directory..."
After:  WARN Using current working directory...

Assisted-by: Cursor
k8s-ci-robot added a commit that referenced this pull request Aug 25, 2025
🐛 (fix) reformat subprocess output for consistent logging during alpha commands (Follow up: #4968)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Migrate from logrus to standard library log/slog

3 participants