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

feat: add global --quiet flag to hide non-error messages #656

Merged
merged 5 commits into from Jan 10, 2024
Merged

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Jan 2, 2024

Closes #644

@phm07 phm07 added the feature label Jan 2, 2024
@phm07 phm07 self-assigned this Jan 2, 2024
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (e1bd800) 45.98% compared to head (1ffa0dc) 46.36%.

Files Patch % Lines
internal/cmd/all/all.go 0.00% 1 Missing ⚠️
internal/cmd/firewall/add_rule.go 50.00% 1 Missing ⚠️
internal/cmd/firewall/delete_rule.go 50.00% 1 Missing ⚠️
internal/cmd/loadbalancer/list.go 50.00% 1 Missing ⚠️
internal/cmd/primaryip/primaryip.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #656      +/-   ##
==========================================
+ Coverage   45.98%   46.36%   +0.38%     
==========================================
  Files         171      179       +8     
  Lines        7514     7877     +363     
==========================================
+ Hits         3455     3652     +197     
- Misses       3616     3765     +149     
- Partials      443      460      +17     

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

@phm07 phm07 marked this pull request as ready for review January 2, 2024 13:58
@phm07 phm07 requested a review from a team as a code owner January 2, 2024 13:58
@phm07 phm07 force-pushed the quiet-flag branch 2 times, most recently from 3fd7062 to 98509d9 Compare January 2, 2024 14:48
@phm07 phm07 changed the title feat: add global --quiet flag to hide progress indicators feat: add global --quiet flag to hide non-error messages Jan 2, 2024
@phm07 phm07 requested a review from jooola January 3, 2024 13:41
@phm07 phm07 marked this pull request as draft January 4, 2024 13:19
@phm07 phm07 force-pushed the quiet-flag branch 2 times, most recently from 2552b2d to 6c84163 Compare January 4, 2024 15:44
@phm07 phm07 marked this pull request as ready for review January 4, 2024 15:45
internal/cli/root.go Outdated Show resolved Hide resolved
Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

Could you refactor all the test to use a test helper and use a list of test cases ?

internal/cmd/base/create_test.go Outdated Show resolved Hide resolved
internal/cmd/base/create_test.go Outdated Show resolved Hide resolved
@phm07 phm07 requested a review from jooola January 4, 2024 17:23
@jooola
Copy link
Member

jooola commented Jan 8, 2024

Is there a chance that we fix the cyclic import by moving this to the main.go:

cli/internal/cli/root.go

Lines 42 to 63 in 2df5e92

cmd.AddCommand(
all.NewCommand(s),
floatingip.NewCommand(s),
image.NewCommand(s),
server.NewCommand(s),
sshkey.NewCommand(s),
version.NewCommand(s),
completion.NewCommand(s),
servertype.NewCommand(s),
context.NewCommand(s),
datacenter.NewCommand(s),
location.NewCommand(s),
iso.NewCommand(s),
volume.NewCommand(s),
network.NewCommand(s),
loadbalancer.NewCommand(s),
loadbalancertype.NewCommand(s),
certificate.NewCommand(s),
firewall.NewCommand(s),
placementgroup.NewCommand(s),
primaryip.NewCommand(s),
)

And maybe move the root command somewhere closer to the base package ?

func NewRootCommand(s state.State) *cobra.Command {

@phm07
Copy link
Contributor Author

phm07 commented Jan 9, 2024

I think we could, but it makes most sense to me to define subcommands where the root command is defined. Also, testing in packages ending with _test is good practice anyway (some linters even enforce it) and we were not consistently doing it before anyway. We could move that into another PR though so that the diff is smaller.

@jooola
Copy link
Member

jooola commented Jan 10, 2024

I think we could, but it makes most sense to me to define subcommands where the root command is defined.

In terms of logic, I understand why, though I don't think it will make it less understandable. But in terms of code structure, I'd rather have a tree/diamond shape than those cyclic imports.

Happy to merge this, though.

@phm07 phm07 merged commit 25fcbbf into main Jan 10, 2024
3 checks passed
@phm07 phm07 deleted the quiet-flag branch January 10, 2024 10:11
apricote pushed a commit that referenced this pull request Feb 1, 2024
## [1.42.0](v1.41.1...v1.42.0) (2024-02-01)


### Features

* add global --quiet flag to hide non-error messages
([#656](#656))
([25fcbbf](25fcbbf)),
closes [#644](#644)
* allow adding/removing multiple labels at once
([#665](#665))
([919c446](919c446)),
closes [#662](#662)
* group subcommands in command help
([#675](#675))
([0cb271f](0cb271f))
* **server:** remove unsupported linux32 rescue type
([#679](#679))
([5bb0350](5bb0350))


### Bug Fixes

* refetch after creating managed certificate
([#685](#685))
([4864553](4864553))
* **server:** fix typo in ip subcommand
([#678](#678))
([c5e3f00](c5e3f00))
* use --poll-interval flag
([#660](#660))
([b9328a6](b9328a6))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

json output format should NOT output progress
2 participants