Skip to content

Implemented global --quiet (-q) flag #516

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

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

Conversation

kanha-gupta
Copy link
Contributor

Fixes #497

The PR is divided in two commits -

  1. centralize the logging. Added a logger.go file in utility package which is being called throughout the codebase, replacing fmt.println() and fmt.printf()
  2. Actual implementation on top of the first commit.

Working demo ->

Screenshot from 2025-02-18 03-33-48
Screenshot from 2025-02-18 03-34-03
Screenshot from 2025-02-18 03-34-20
Screenshot from 2025-02-18 03-46-39

@kanha-gupta
Copy link
Contributor Author

Hi @fernando-villalba @uzaxirr please review this :)

@giornetta giornetta self-requested a review March 13, 2025 10:24
@giornetta giornetta added the enhancement New feature or request label May 6, 2025
@giornetta
Copy link
Member

Hi @kanha-gupta! Thanks for the contribution and sorry for the delay! We will review this shortly 😄

Unfortunately there are some conflicts with the master branch: you can go ahead and resolve them if you're still interested in contributing, otherwise we can take it from here! 👍

Thanks again!

@fulviodenza fulviodenza self-requested a review May 6, 2025 09:11
@kanha-gupta
Copy link
Contributor Author

Hi @giornetta can you take a look at the approach I have taken ? if you like the approach, I'll go ahead and resolve conflicts.
Thanks

@giornetta
Copy link
Member

@kanha-gupta I personally like the approach. @fulviodenza @uzaxirr what's your take?

@uzaxirr
Copy link
Contributor

uzaxirr commented May 12, 2025

This approach seems fine. but my only concern is too many files being modified in this PR. Can you divide them?

Copy link
Member

@alessandroargentieri alessandroargentieri left a comment

Choose a reason for hiding this comment

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

LGTM

@alessandroargentieri
Copy link
Member

this MR changes the Printing of the output nesting the --quiet logic, so I cannot imagine a way to divide these files, correct me if I'm wrong @uzaxirr

@uzaxirr
Copy link
Contributor

uzaxirr commented May 12, 2025

@alessandroargentieri We can divide this in Multiple PRs, first just a PR to introduce logger in utility and changeing just 2-3 files. and then PRs to change them in resources, ideally i would say one PR for 2 resources.

@kanha-gupta
Copy link
Contributor Author

Hi @giornetta @alessandroargentieri @uzaxirr Thanks for the review !
I can surely divide this PR into smaller ones. just let me know if I should open all the PRs at the same time or one by one.
I'll proceed upon your confirmation.
Thanks once again !

@alessandroargentieri
Copy link
Member

alessandroargentieri commented May 12, 2025

Not necessary. Me and @uzaxirr clarified on on that point and agreed on merging if you solve the conflicts. Indeed, since it's a single feature, dividing into more than one PR means fragmenting the single feature which is something potentially introducing some refuses.

Copy link
Member

@alessandroargentieri alessandroargentieri left a comment

Choose a reason for hiding this comment

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

LGTM

@kanha-gupta
Copy link
Contributor Author

Not necessary. Me and @uzaxirr clarified on on that point and agreed on merging if you solve the conflicts. Indeed, since it's a single feature, dividing into more than one PR means fragmenting the single feature which is something potentially introducing some refuses.

@alessandroargentieri Thanks for the info. I have resolved merge conflicts. you can merge now :)

@kanha-gupta kanha-gupta force-pushed the implement_quiet branch 2 times, most recently from e18f3ae to 42afc3b Compare May 12, 2025 19:20
@giornetta
Copy link
Member

I would make sure that every fmt.Printf or other printing functions are replaced with this PR before merging it.

I suppose many new features have been added since this PR was opened, so some new functions may not use this new approach

@alessandroargentieri
Copy link
Member

I wish a rebase has been done and a search all functionality has been used. Isn't it @kanha-gupta ?

@kanha-gupta kanha-gupta force-pushed the implement_quiet branch 2 times, most recently from 268402d to a0ee675 Compare May 12, 2025 21:17
@kanha-gupta
Copy link
Contributor Author

kanha-gupta commented May 12, 2025

I would make sure that every fmt.Printf or other printing functions are replaced with this PR before merging it.

I suppose many new features have been added since this PR was opened, so some new functions may not use this new approach

I would love that too ! unfortunately, I had to leave few modules due to "import cycle" issue during development.
These are the following :

  1. config/config.go
  2. common/common.go
  3. cmd/loadbalancer/loadbalancer_remove.go.disabled - left it because it's already disabled, might be useful for future reference ?

Except from these, fmt.Print is refactored in every file. Figured out that I'll have to create a new module just for a logger file so skipped these two.
I would appreciate a better approach from your side :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We need a global --quiet (-q) flag
4 participants