Skip to content

Use CLI11 to provide proper argument/option parsing to netdatacli #19099

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vkalintiris
Copy link
Contributor

Summary

SSIA.

Test Plan
  • CI jobs.
Additional Information

Needs more work to update references of netdatacli throughout the codebase.

@vkalintiris vkalintiris self-assigned this Nov 28, 2024
@github-actions github-actions bot added area/daemon area/build Build system (autotools and cmake). labels Nov 28, 2024
Copy link
Member

@Ferroin Ferroin left a comment

Choose a reason for hiding this comment

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

Largely LGTM other than the couple of review comments.

)
else()
FetchContent_Declare(CLI11
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
Copy link
Member

Choose a reason for hiding this comment

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

Mimalloc should be it’s own thing unless it’s absolutely required by CLI11.

Comment on lines +152 to +167
app.set_help_flag("--help", "Show this help message and exit");
app.add_flag("--reload-health", reload_health, "Reload health configuration");
app.add_flag("--reload-labels", reload_labels, "Reload all labels");
app.add_flag("--reopen-logs", reopen_logs, "Close and reopen log files");
app.add_flag("--shutdown-agent", shutdown_agent, "Cleanup and exit the netdata agent");
app.add_flag("--fatal-agent", fatal_agent, "Log the state and halt the netdata agent");
app.add_flag("--reload-claiming-state", reload_claiming_state, "Reload agent claiming state from disk");
app.add_flag("--ping", ping, "Return with 'pong' if agent is alive");
app.add_option(
"--aclk-state", aclk_state, "Returns current state of ACLK and Cloud connection. Use 'json' for JSON format");
app.add_flag("--dump-config", dumpconfig, "Returns the current netdata.conf on stdout");
app.add_option(
"--remove-stale-node",
remove_stale_node,
"Unregisters and removes a node from the cloud. Specify node_id, machine_guid, hostname, or ALL_NODES");
app.add_flag("--version", version, "Returns the netdata version");
Copy link
Member

Choose a reason for hiding this comment

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

Changing all of these to use a -- prefix is a breaking change. Ideally we need to keep the old syntax without the -- to avoid breaking things (though I would like to see --help supported alongside help, and probably ideally also a --version option since we should be able to provide that).

Comment on lines +2644 to +2662
include(FetchContent)
include(NetdataFetchContentExtra)

if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.28)
FetchContent_Declare(
CLI11
GIT_REPOSITORY https://github.com/CLIUtils/CLI11.git
GIT_TAG 6c7b07a878ad834957b98d0f9ce1dbe0cb204fc9 # v2.4.2
EXCLUDE_FROM_ALL
)
else()
FetchContent_Declare(CLI11
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
GIT_REPOSITORY https://github.com/CLIUtils/CLI11.git
GIT_TAG 6c7b07a878ad834957b98d0f9ce1dbe0cb204fc9 # v2.4.2
)
endif()

FetchContent_MakeAvailable_NoInstall(CLI11)
Copy link
Member

Choose a reason for hiding this comment

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

I thought our goal was to minimize the amount of stuff like this in the main CMakeLists.txt file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build system (autotools and cmake). area/daemon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants