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: Migrate to urfave/cli #11700

Merged
merged 29 commits into from
Aug 25, 2022
Merged

feat: Migrate to urfave/cli #11700

merged 29 commits into from
Aug 25, 2022

Conversation

sspaink
Copy link
Contributor

@sspaink sspaink commented Aug 17, 2022

related to: #11316

There is a goal to clean up the flags and commands of Telegraf (#11316), to help that effort I've migrated the project from using the standard library flag package to using urfave/cli. The urfave/cli package offers a clean and straightforward way to implement flags and commands. I believe this will help provide and easy foundation to begin updating and maintaining the flags/commands.

The goal of this pull request is to just begin using "urfave/cli" package, with no breaking changes, so everything should work as it does now. Then in future pull requests, we can begin deprecating and introducing new flags/commands. I've also added test coverage around the flag/commands which didn't exist before. To add the test coverage I moved the main business logic to a separate function called runApp where the actual agent, pprof, and config get passed in. These are mocked out in the new tests.

The flags --sample-config and --version have already been marked as deprecated because there are duplicate commands that already exists telegraf config and telegraf version. I also marked --plugin-directory as deprecated as this is a feature that was quickly disabled after introduction and would require a special build of Telegraf to get working that is not being released officially.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 17, 2022
@sspaink sspaink force-pushed the cliv2 branch 2 times, most recently from 641882d to 6d59298 Compare August 17, 2022 20:29
@sspaink sspaink added refactor size/l 1 week or more effort labels Aug 17, 2022
@sspaink sspaink marked this pull request as ready for review August 22, 2022 19:59
@sspaink sspaink requested a review from a team August 22, 2022 20:02
@powersj
Copy link
Contributor

powersj commented Aug 23, 2022

Huge thank you for this. It will make things much more consistent and more easily make changes down the road. Some initial questions for our chat today after quickly scanning this PR:

  1. Windows only flags now show up in linux output, is there a way to filter these? I don't find this as a blocker
  2. Tested on mac and windows? What can I do to help?
  3. Why were the internal version variables made public?
  4. What prompted the pprof changes?

@sspaink
Copy link
Contributor Author

sspaink commented Aug 23, 2022

1. Windows only flags now show up in linux output, is there a way to filter these? I don't find this as a blocker
Good catch, this is fixable
2. Tested on mac and windows? What can I do to help?
Windows services and Mac needs to be tested
3. Why were the internal version variables made public?
For testing, https://github.com/influxdata/telegraf/blob/cliv2/cmd/telegraf/telegraf_test.go#L314-L357 using it to set the version to something predictable.
4. What prompted the pprof changes?
For testing and err channel was added to avoid calling panic if the pprof fails

Action items from discussion:

  1. Test windows
  2. Test mac
  3. Test error conditions (bad config ,etc)
  4. Test version
  5. Test sample config with diff
  6. Add tests for all flags
  7. Consider different name for "AgentManager"

@powersj
Copy link
Contributor

powersj commented Aug 23, 2022

Test sample config with diff

This looks good no diff between the file generated and the file from master

Test version

Once I added your Makefile changes locally, I think the only difference is the lack of a newline.

# telegraf from your branch
❯ ./telegraf version | od -c
0000000   T   e   l   e   g   r   a   f       1   .   2   4   .   0   -
0000020   a   a   b   9   5   4   a   8       (   g   i   t   :       c
0000040   l   i   v   2   @   a   a   b   9   5   4   a   8   )
0000056
# this is telegraf built from master
❯ /tmp/telegraf version | od -c
0000000   T   e   l   e   g   r   a   f       1   .   2   4   .   0   -
0000020   9   e   a   2   d   e   a   c       (   g   i   t   :       m
0000040   a   s   t   e   r   @   9   e   a   2   d   e   a   c   )  \n
0000060

@sspaink
Copy link
Contributor Author

sspaink commented Aug 23, 2022

@powersj I've added the missing newline, nice find!

@sspaink
Copy link
Contributor Author

sspaink commented Aug 23, 2022

I've tested windows service by running the following commands:

telegraf.exe --service install	Install telegraf as a service
telegraf.exe --service uninstall	Remove the telegraf service
telegraf.exe --service start	Start the telegraf service
telegraf.exe --service stop	Stop the telegraf service

With a simple config:

[[inputs.cpu]]

[[outputs.file]]
    files = ["H:\\Sandbox\\test.txt"]

There was an issue I resolved in the latest commit, were I had added an error channel that was blocking the service from starting. I replaced it with just printing an error message.

The struct AgentManager has also been renamed to Telegraf to prevent overloading the word Agent which already has a meaning in this project.

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

I completed my testing on macOS, went through each of the options, and ensured they worked as I understood them.

On Linux, I tried several invalid configs and options to ensure we still error as expected.

I have put a few comments below, primarily nits and clean-up.

cmd/telegraf/main.go Outdated Show resolved Hide resolved
cmd/telegraf/main.go Outdated Show resolved Hide resolved
cmd/telegraf/main.go Outdated Show resolved Hide resolved
cmd/telegraf/main.go Outdated Show resolved Hide resolved
cmd/telegraf/telegraf_posix.go Outdated Show resolved Hide resolved
cmd/telegraf/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

+1

Looks great, and makes it much easier for us to maintain the CLI going forward, as well as extend it with additional subcommand and flags.

My only concern was re: internal variables, but we can chat as a team about that and make changes before v1.24.0 if needed.

cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved
cmd/telegraf/telegraf.go Show resolved Hide resolved
cmd/telegraf/main.go Outdated Show resolved Hide resolved
@telegraf-tiger
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin size/l 1 week or more effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants