Added basic version of quiet mode to service create command #1904

sreeram-venkitesh · 2023-12-07T13:10:27Z

Description

This PR was made as part of knative 48h hack. This is a proof of concept and there is no need to merge it until the work is complete.

Changes

Added QuietMode flag to root command
Used QuietMode flag to conditionally print/hide output for the kn service create command

Reference

Proof of concept for #182

knative-prow · 2023-12-07T13:10:37Z

Welcome @sreeram-venkitesh! It looks like this is your first PR to knative/client 🎉

knative-prow · 2023-12-07T13:10:38Z

Hi @sreeram-venkitesh. Thanks for your PR.

I'm waiting for a knative 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/test-infra repository.

knative-prow

@sreeram-venkitesh: 0 warnings.

In response to this:

Description

Changes

Added QuietMode flag to root command

Used QuietMode flag to conditionally print/hide output for the kn service create command

Reference

Proof of concept for #182

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/test-infra repository.

dsimansk · 2023-12-07T13:24:20Z

/ok-to-test

codecov · 2023-12-07T13:28:25Z

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 79.19%. Comparing base (7b11654) to head (902dea9).
Report is 23 commits behind head on main.

❗ Current head 902dea9 differs from pull request most recent head ea2b02b. Consider uploading reports for the commit ea2b02b to get more accurate results

Files	Patch %	Lines
pkg/kn/commands/service/create.go	50.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1904      +/-   ##
==========================================
- Coverage   79.20%   79.19%   -0.02%     
==========================================
  Files         180      180              
  Lines       14075    14081       +6     
==========================================
+ Hits        11148    11151       +3     
- Misses       2155     2156       +1     
- Partials      772      774       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/kn/root/root.go

dsimansk · 2023-12-08T13:11:07Z

It seems --quiet flag is conflicting in some of the unit tests. Aka it's working and discarding the outputs.

dsimansk · 2023-12-08T13:23:33Z

pkg/kn/commands/logger/logger.go

+type Config struct {
+	QuietMode bool
+}


Config struct should accept io.Writer as well. There might be other use cases that benefit from output redirect, not always being os.Stdout. Then it should be initialized with value of:

cmd.OutOrStdout()

That's the case for failing unit tests, because output is redirected and captured to assert.

IMO this still seems like a problematic part, at least for unit tests. @sreeram-venkitesh pls let me know if you can take a look at it. Thanks!

dsimansk · 2023-12-08T13:24:02Z

pkg/kn/commands/logger/logger.go

+type Config struct {
+	QuietMode bool
+}


That's the case for failing unit tests, because output is redirected and captured to assert.

rhuss · 2023-12-08T14:48:40Z

pkg/kn/commands/logger/logger.go

+	QuietMode bool
+}
+
+func NewLogger(config Config) io.Writer {


Instead of returning a writer, I probably would initialize a global variable here, that stores the write, so that subsequent calls to Log would need to pass back in the Writer again. E.g rename this just to func InitLogger(conf Config) { ... (without a return value) then store a static variable outWriter and use that outWriter in the Log() method directly.

rhuss · 2023-12-08T14:49:49Z

pkg/kn/commands/logger/logger.go

+	return io.Writer(os.Stdout)
+}
+
+func Log(writer io.Writer, message string) {


I would change this to Log(fmt string, args interface{}) string to match the signature of Fprintf() (which is also a vararg command, please check its signature). Then use the initialized outWriter from above directly in this Log method.

knative-prow · 2023-12-17T20:07:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sreeram-venkitesh
Once this PR has been reviewed and has the lgtm label, please ask for approval from dsimansk. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

sreeram-venkitesh · 2023-12-20T14:28:05Z

/retest

knative-prow · 2023-12-20T15:00:27Z

@sreeram-venkitesh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
unit-tests_client_main	`ea2b02b`	link	true	`/test unit-tests`
integration-tests_client_main	`ea2b02b`	link	true	`/test integration-tests`
integration-tests-latest-release_client_main	`ea2b02b`	link	true	`/test integration-tests-latest-release`

Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

github-actions · 2024-04-09T01:26:59Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

sreeram-venkitesh · 2024-04-09T13:04:38Z

/remove-lifecycle stale

rhuss · 2024-04-29T06:52:13Z

@sreeram-venkitesh I'm so sorry that I'm not able to work on these (and oer client-related) issues currently, because of other work assignments. I can't forsee when this will change, but maybe @dsimansk or others can help out ?

What are currently the next steps needed to get this landed ?

Added basic version of quiet mode to service create command

902dea9

knative-prow bot requested review from creydr, dprotaso and itsmurugappan December 7, 2023 13:10

knative-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 7, 2023

knative-prow bot reviewed Dec 7, 2023

View reviewed changes

knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 7, 2023

knative-prow bot 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 Dec 7, 2023

Wrapped common functionality into Logger

e4596c6

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 8, 2023

dsimansk reviewed Dec 8, 2023

View reviewed changes

pkg/kn/root/root.go Outdated Show resolved Hide resolved

Regen docs

257a4d9

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 8, 2023

dsimansk reviewed Dec 8, 2023

View reviewed changes

dsimansk requested changes Dec 8, 2023

View reviewed changes

knative-prow bot assigned dsimansk Dec 8, 2023

dsimansk added the knative48h Issue selected for the "48h Knative" hackathon label Dec 8, 2023

rhuss reviewed Dec 8, 2023

View reviewed changes

Renamed quiet-mode to quiet

4770423

Added io.Writer to Logger config

ea2b02b

dprotaso removed their request for review January 9, 2024 13:28

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2024

knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added basic version of quiet mode to service create command #1904

Added basic version of quiet mode to service create command #1904

sreeram-venkitesh commented Dec 7, 2023 •

edited

knative-prow bot commented Dec 7, 2023

knative-prow bot commented Dec 7, 2023

knative-prow bot left a comment

dsimansk commented Dec 7, 2023

codecov bot commented Dec 7, 2023 •

edited

dsimansk commented Dec 8, 2023

dsimansk Dec 8, 2023

dsimansk Dec 8, 2023

dsimansk Dec 20, 2023

dsimansk Dec 8, 2023

rhuss Dec 8, 2023

rhuss Dec 8, 2023

knative-prow bot commented Dec 17, 2023

sreeram-venkitesh commented Dec 20, 2023

knative-prow bot commented Dec 20, 2023

github-actions bot commented Apr 9, 2024

sreeram-venkitesh commented Apr 9, 2024

rhuss commented Apr 29, 2024

Added basic version of quiet mode to service create command #1904

Are you sure you want to change the base?

Added basic version of quiet mode to service create command #1904

Conversation

sreeram-venkitesh commented Dec 7, 2023 • edited

Description

Changes

Reference

knative-prow bot commented Dec 7, 2023

knative-prow bot commented Dec 7, 2023

knative-prow bot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

dsimansk commented Dec 7, 2023

codecov bot commented Dec 7, 2023 • edited

Codecov Report

dsimansk commented Dec 8, 2023

dsimansk Dec 8, 2023

Choose a reason for hiding this comment

dsimansk Dec 8, 2023

Choose a reason for hiding this comment

dsimansk Dec 20, 2023

Choose a reason for hiding this comment

dsimansk Dec 8, 2023

Choose a reason for hiding this comment

rhuss Dec 8, 2023

Choose a reason for hiding this comment

rhuss Dec 8, 2023

Choose a reason for hiding this comment

knative-prow bot commented Dec 17, 2023

sreeram-venkitesh commented Dec 20, 2023

knative-prow bot commented Dec 20, 2023

github-actions bot commented Apr 9, 2024

sreeram-venkitesh commented Apr 9, 2024

rhuss commented Apr 29, 2024

sreeram-venkitesh commented Dec 7, 2023 •

edited

codecov bot commented Dec 7, 2023 •

edited