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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sreeram-venkitesh
Copy link

@sreeram-venkitesh sreeram-venkitesh commented Dec 7, 2023

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

Copy link

knative-prow bot commented Dec 7, 2023

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

@knative-prow 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
Copy link

knative-prow bot commented Dec 7, 2023

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.

Copy link

@knative-prow knative-prow bot left a comment

Choose a reason for hiding this comment

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

@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.

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

dsimansk commented Dec 7, 2023

/ok-to-test

@knative-prow 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
Copy link

codecov bot commented Dec 7, 2023

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.

@knative-prow 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
pkg/kn/root/root.go Outdated Show resolved Hide resolved
@knative-prow 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
Copy link
Contributor

dsimansk commented Dec 8, 2023

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

Comment on lines 25 to 29
type Config struct {
QuietMode bool
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Comment on lines 25 to 29
type Config struct {
QuietMode bool
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

func NewLogger(config Config) io.Writer {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return io.Writer(os.Stdout)
}

func Log(writer io.Writer, message string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

knative-prow bot commented Dec 17, 2023

[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:

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

@sreeram-venkitesh
Copy link
Author

/retest

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

github-actions bot commented Apr 9, 2024

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.

@github-actions 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
@sreeram-venkitesh
Copy link
Author

/remove-lifecycle stale

@knative-prow 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
@rhuss
Copy link
Contributor

rhuss commented Apr 29, 2024

@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 ?

Copy link

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.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2024
@rhuss
Copy link
Contributor

rhuss commented Jul 30, 2024

/remove-lifecycle stale

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 30, 2024
Copy link

knative-prow bot commented Sep 1, 2024

@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 6b64b5e link true /test unit-tests
integration-tests-latest-release_client_main 6b64b5e link true /test integration-tests-latest-release
integration-tests_client_main 6b64b5e link true /test integration-tests

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

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2024
@knative-prow-robot
Copy link
Contributor

PR needs rebase.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
knative48h Issue selected for the "48h Knative" hackathon needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants