Skip to content

[KAAP-545] Add clean msg to byohctl verbosity flag - #241

Merged
snslk merged 3 commits into
mainfrom
private/snh/kaap-545
Aug 26, 2026
Merged

[KAAP-545] Add clean msg to byohctl verbosity flag#241
snslk merged 3 commits into
mainfrom
private/snh/kaap-545

Conversation

@snslk

@snslk snslk commented Aug 24, 2026

Copy link
Copy Markdown

Bug

Ambiguous error message , user expecting -v with no argument to work since error msg shows default minimal .

Fix

Clean message listing required options and mentioning omitting the flag still shows minimal verbose by default .

Testing

Before

root@byoh-snh-1:~# byohctl deauthorise -v
Error: flag needs an argument: 'v' in -v
Usage:
  byohctl deauthorise [flags]

Examples:
  byohctl deauthorise -v all

Flags:
  -h, --help               help for deauthorise
  -v, --verbosity string   Log verbosity level (all, important, minimal, critical, none) (default "minimal")

[2026-08-24 10:21:25] [ERROR] Command execution failed: flag needs an argument: 'v' in -v
root@byoh-snh-1:~# byohctl decommission -v  
Error: flag needs an argument: 'v' in -v
Usage:
  byohctl decommission [flags]

Examples:
  byohctl decommission -v all

Flags:
  -h, --help               help for decommission
  -v, --verbosity string   Log verbosity level (all, important, minimal, critical, none) (default "minimal")

[2026-08-24 10:22:19] [ERROR] Command execution failed: flag needs an argument: 'v' in -v

After

root@byoh-snh-1:~# ./byohctl decommission -v
Error: flag needs an argument: 'v' in -v
Usage:
  byohctl decommission [flags]

Examples:
  byohctl decommission -v all

Flags:
  -h, --help               help for decommission
  -v, --verbosity string   Log verbosity level. Requires one of: all, important, minimal, critical.
                           Omitting the flag will show minimal verbosity (default "minimal")

Global Flags:
      --insecure   Skip TLS certificate verification for management plane API calls. Also recorded in the host agent's kubeconfig, so it persists after onboarding. Intended for on-prem deployments serving a self-signed or private-CA certificate.

[2026-08-25 07:36:51] [ERROR] Command execution failed: flag needs an argument: 'v' in -v
root@byoh-snh-1:~# ./byohctl decommission   
[2026-08-25 07:37:00] [WARNING] running from /root/byohctl, not /usr/bin/byohctl. Use `byohctl` (via PATH) instead of this copy.
[2026-08-25 07:37:00] [SUCCESS] Successfully retrieved Kubernetes client
[2026-08-25 07:37:00] [SUCCESS] Successfully retrieved ByoHosts object from the management plane
[2026-08-25 07:37:00] [SUCCESS] Successfully deleted ByoHosts object
[2026-08-25 07:37:01] [SUCCESS] Successfully purged Debian package pf9-byohost-agent
[2026-08-25 07:37:01] [SUCCESS] Successfully ran dpkg purge
[2026-08-25 07:37:01] [SUCCESS] Successfully decommissioned host from the pf9 kaapi management cluster
root@byoh-snh-1:~# 

@snslk snslk changed the title [KAAP-545] default value for -v not working setting -v to pick all by default [KAAP-545] Fix default value of byohctl cmd verbosity Aug 24, 2026
@snslk
snslk requested a review from a team August 24, 2026 11:11

@sebastian-pf9 sebastian-pf9 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

unit tests need fixing

Ginkgo ran 11 suites in 4m43.949132652s
Test Suite Passed
cd cmd && go test ./...
? github.com/platform9/cluster-api-provider-bringyourownhost/cmd/byohctl [no test files]
ok github.com/platform9/cluster-api-provider-bringyourownhost/cmd/byohctl/client 0.046s
--- FAIL: TestOnboardFlags (0.00s)
onboard_test.go:102: Expected verbosity 'debug', got 'all'
--- FAIL: TestConfigFilePrecedence (0.00s)
--- FAIL: TestConfigFilePrecedence/CLI_overrides_config (0.00s)
onboard_test.go:334: Expected verbosity = 'debug', got 'all'

@sebastian-pf9 sebastian-pf9 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

actually - you tested the change. add it to the unit tests please

root@byoh-snh-1:~# ./byohctl deauthorise -v
[2026-08-24 10:21:33] [WARNING] running from /root/byohctl, not /usr/bin/byohctl. Use `byohctl` (via PATH) instead of this copy.
[2026-08-24 10:21:33] [INFO] Performing deauthorise operation for host in namespace test-du-ciprsmoke-5041227-default-service
[2026-08-24 10:21:33] [SUCCESS] Successfully retrieved Kubernetes client
[2026-08-24 10:21:33] [SUCCESS] Successfully retrieved ByoHosts object from the management plane
Info: Machine deployment replica count is 1. This is the last node in the cluster.
Do you want to continue with de-auth? (y/n): y
[2026-08-24 10:21:40] [SUCCESS] Successfully annotated machine object that needs to be removed from the cluster
[2026-08-24 10:21:40] [SUCCESS] Successfully scaled down machine deployment by 1
[2026-08-24 10:21:40] [INFO] Waiting for machineRef to be unset...
[2026-08-24 10:21:45] [INFO] Waiting for machineRef to be unset...
[2026-08-24 10:21:50] [INFO] Waiting for machineRef to be unset...
[2026-08-24 10:21:55] [SUCCESS] MachineRef unset
[2026-08-24 10:21:55] [SUCCESS] MachineRef successfully unset for the host
[2026-08-24 10:21:55] [SUCCESS] Successfully deauthorised host from the byo cluster

root@byoh-snh-1:~# ./byohctl decommission -v
[2026-08-24 10:22:24] [WARNING] running from /root/byohctl, not /usr/bin/byohctl. Use `byohctl` (via PATH) instead of this copy.
[2026-08-24 10:22:24] [INFO] Performing decommission operation for host in namespace test-du-ciprsmoke-5041227-default-service
[2026-08-24 10:22:24] [SUCCESS] Successfully retrieved Kubernetes client
[2026-08-24 10:22:24] [SUCCESS] Successfully retrieved ByoHosts object from the management plane
[2026-08-24 10:22:24] [INFO] MachineRef is not set to the byohost object. Host is not part of any cluster. Deleting the byohost object and running dpkg purge.
[2026-08-24 10:22:24] [INFO] Deleting ByoHosts object and running dpkg purge
[2026-08-24 10:22:24] [SUCCESS] Successfully deleted ByoHosts object
[2026-08-24 10:22:25] [SUCCESS] Successfully purged Debian package pf9-byohost-agent
[2026-08-24 10:22:25] [SUCCESS] Successfully ran dpkg purge
[2026-08-24 10:22:25] [SUCCESS] Successfully decommissioned host from the pf9 kaapi management cluster
root@byoh-snh-1:~# 

@indradhanush indradhanush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking at this, I'm not sure we should implement this default to be honest. A user may expect -v's default to be the least verbose level, vs another may expect it to be the maximum verbosity. What I'm trying to imply is: That finding the right default for a flag like verbosity does not appear to be that straighforward. And the additional complexity with the flag parsing and defaults does not seem to be worth it.

Please reconsider. Not a request changes / approve because I don't have the strongest of opinion about this either way at this point.

If the team thinks this is a good idea, let's go with it, otherwise not.

Comment thread cmd/byohctl/cmd/onboard_test.go Outdated
…mit -v/--verbosity completely which shows minimal by default
@snslk
snslk force-pushed the private/snh/kaap-545 branch from 94ae694 to fe2c6e7 Compare August 25, 2026 07:42
@snslk

snslk commented Aug 25, 2026

Copy link
Copy Markdown
Author

Looking at this, I'm not sure we should implement this default to be honest. A user may expect -v's default to be the least verbose level, vs another may expect it to be the maximum verbosity. What I'm trying to imply is: That finding the right default for a flag like verbosity does not appear to be that straighforward. And the additional complexity with the flag parsing and defaults does not seem to be worth it.

Please reconsider. Not a request changes / approve because I don't have the strongest of opinion about this either way at this point.

If the team thinks this is a good idea, let's go with it, otherwise not.

Agree to your point , this issue doesn't require this much changes , just adding cleaner msg to user in case they try to run -v without any argument , changed testing notes as per new changes .
cc: @indradhanush

@snslk

snslk commented Aug 25, 2026

Copy link
Copy Markdown
Author

actually - you tested the change. add it to the unit tests please

@sebastian-pf9, No unit tests required here , removed unnecessary logical change instead added cleaner user message.

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@indradhanush indradhanush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The help text needs fixing. Please see comments.

Comment thread cmd/byohctl/cmd/deauthorise.go Outdated
Comment thread cmd/byohctl/cmd/decommission.go Outdated
Comment thread cmd/byohctl/cmd/onboard.go Outdated
@indradhanush

Copy link
Copy Markdown
Collaborator

@snslk Also can you please update the PR title and desccription to match what we're doing now?

Comment thread cmd/byohctl/cmd/onboard.go Outdated
@snslk
snslk requested a review from indradhanush August 25, 2026 11:22
@snslk snslk changed the title [KAAP-545] Fix default value of byohctl cmd verbosity [KAAP-545] Add clean msg to byohctl verbosity flag Aug 25, 2026

@indradhanush indradhanush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for taking the time to revise this!

@snslk
snslk merged commit 2a4aa68 into main Aug 26, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants