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

We should refactor the agent cli output #311

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

huchen2021
Copy link
Contributor

@huchen2021 huchen2021 commented Jan 12, 2022

Fixes #253

There are a lot of wrong flag for byoh-hostagent

  1. The following flags belong to "cli", not " byoh-hostagent". It should put all flag function in main function of "cli", not as global variable.
  • -bundle-repo string
  • -cache-path string
  • -detect
  • -install
  • -k8s string
  • -list-supported
  • -os string
  • -preview-os-changes
  • -tag string
  • -uninstall
  1. The following flags belong to klog which we don't need it anymore
  • -add_dir_header
  • -alsologtostderr
  • -log_backtrace_at value
  • -log_dir string
  • -log_file string
  • -log_file_max_size uint
  • -logtostderr
  • -skip_headers
  • -skip_log_headers
  • -stderrthreshold value
  • -v value
  • -vmodule value

After fix this, byoh-hostagent only has the following flags:

  • -downloadpath string
  • -kubeconfig string
  • -label value
  • -metricsbindaddress string
  • -namespace string
  • -skip-installation

Plus, In order to avoid such a bug again, I add a test for --help command. Any unexpected option will caused the test failure.

Copy link
Contributor

@anusha94 anusha94 left a comment

Choose a reason for hiding this comment

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

Can we write a test for -h or --help command?

@huchen2021
Copy link
Contributor Author

huchen2021 commented Jan 13, 2022

Can we write a test for -h or --help command?

Yes, it will help us to avoid such a bug again. Done. Please review it again.

@huchen2021 huchen2021 requested a review from anusha94 January 13, 2022 07:20
@huchen2021 huchen2021 requested a review from anusha94 January 13, 2022 09:16
Copy link
Contributor

@anusha94 anusha94 left a comment

Choose a reason for hiding this comment

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

lgtm

@huchen2021 huchen2021 requested a review from anusha94 January 14, 2022 02:45
Copy link
Contributor

@dharmjit dharmjit left a comment

Choose a reason for hiding this comment

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

LGTM.

Could we keep all flag-related tests in a single test file.?

agent/help_flag_test.go Show resolved Hide resolved
@huchen2021
Copy link
Contributor Author

huchen2021 commented Jan 14, 2022

LGTM.

Could we keep all flag-related tests in a single test file.?

We already have label_flag_test.go. And If we put label_flag_test.go and help_flag_test.go together, it would be huge file. And if we have any other flag test in the future, and all put in one file, it will became a super huge file. Don't think it is a good idea.

@huchen2021 huchen2021 requested a review from dharmjit January 14, 2022 03:34
@dharmjit
Copy link
Contributor

We already have label_flag_test.go. And If we put label_flag_test.go and help_flag_test.go together

Again no strong opinions, but IMO we are keeping a single test file for a package, here for agent main we split that into 2 files agent_test.go and label_flag_test.go as the functionalities are different and it does not require envtest/k8s client, etc.

@huchen2021 huchen2021 merged commit a01750c into vmware-tanzu:main Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We should refactor the agent cli output
5 participants