Skip to content

fix: invalid config for vsphere#7087

Closed
NguyenHoangSon96 wants to merge 4 commits into
influxdata:masterfrom
NguyenHoangSon96:fix/invalid-config-vsphere
Closed

fix: invalid config for vsphere#7087
NguyenHoangSon96 wants to merge 4 commits into
influxdata:masterfrom
NguyenHoangSon96:fix/invalid-config-vsphere

Conversation

@NguyenHoangSon96

@NguyenHoangSon96 NguyenHoangSon96 commented Oct 22, 2025

Copy link
Copy Markdown
Contributor

Closes 26912

  • My steps to fix:
  1. Check out the source telegraf to my local.
  2. Run go run ./cmd/telegraf config > etc/telegraf.conf command to generate etc/telegraf.conf file.
  3. Tweak the src/writeData/utils/updateTelegrafPlugins.mjs so it will get contents from my local influxdata/telegra/etc/telegraf.conf instead of the remote repository /influxdata/telegraf.
  4. Run telegraf-plugins:update command to generate new contents for md and conf files.
  5. Create this PR.

This is just a very primitive implementation of the updateTelegrafPlugins.mjs to make the script work
updateTelegrafPlugins.txt

  • Some thoughts:
  1. I don't know how these things should work exactly; this is just my assumption. If my fixes are not correct, then I will just manually remove the - at the beginning of the .conf file.
  2. If this is correct, then we should update the src/writeData/utils/README.md guidance and the src/writeData/utils/updateTelegrafPlugins.mjs script file to get contents from etc/telegraf.conf in the local repository, not the remote.
  • Before the fix, there is a - at the beginning of the file.
Screen.Recording.2025-10-22.at.09.55.53.mov
  • After I modify src/writeData/utils/updateTelegrafPlugins.mjs and run the command telegraf-plugins:update, the - at the beginning of the file disappears.
Screen.Recording.2025-10-22.at.10.01.23.mov

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Oct 22, 2025
@NguyenHoangSon96 NguyenHoangSon96 requested review from a team as code owners October 22, 2025 03:18
@NguyenHoangSon96 NguyenHoangSon96 requested review from philjb and removed request for philjb October 22, 2025 03:28
@NguyenHoangSon96

NguyenHoangSon96 commented Oct 23, 2025

Copy link
Copy Markdown
Contributor Author
  • I edited the updateTelegrafPlugins.mjs a little bit so it only modifies the ".conf" and ".md" of the vsphere file.
  • But I don't know, should we need to generate contents for the rest of the plugins ".conf" and ".md" files? Looks like they are all out of sync compared to "influxdata/telegraf/${version}/etc/telegraf.conf". Maybe we should do it in another PR 🤷

@philjb

philjb commented Oct 23, 2025

Copy link
Copy Markdown
Contributor
  • I edited the updateTelegrafPlugins.mjs a little bit so it only modifies the ".conf" and ".md" of the vsphere file.
  • But I don't know, should we need to generate contents for the rest of the plugins ".conf" and ".md" files? Looks like they are all out of sync compared to "influxdata/telegraf/${version}/etc/telegraf.conf". Maybe we should do it in another PR 🤷

I would regenerate all of them. You can do that in a separate pr if you like.

I will approve this pr since it is only touching statically served files and I assume it imported them as they are in the telegraf repo.

@philjb philjb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM -- statically served files; i skimmed them and the changes look ok; but i did not diff them with the source files in the telegraf repo. @NguyenHoangSon96 -- to confirm the import script is working, you could diff the conf and md file with the source ones in telegraf's repo.

@NguyenHoangSon96 NguyenHoangSon96 mentioned this pull request Oct 28, 2025
5 tasks
@wdoconnell

Copy link
Copy Markdown
Contributor

I may be missing some context here. Based on the original issue, do we need changes other than just removing the dash? This looks like more changes than that.

@NguyenHoangSon96 Please also update the PR to merge into master (not influxdata:master), then amend and re-push to rerun the CI pipeline.

Before enabling merge I would also address @philjb's comment here.

@philjb

philjb commented Nov 4, 2025

Copy link
Copy Markdown
Contributor

I may be missing some context here. Based on the original issue, do we need changes other than just removing the dash? This looks like more changes than that.

Yes and no -- we could just remove the dash but the dash is there because these configs are programmatically generated from the telegraf repo and at the time this conf was generated, the dash was in the telegraf config (incorrectly).

The other changes in this pr bring this telegraf plugin conf up to date with the latest conf in the telegraf repo.

It's been a long time since these were sync'd. Gunnar used to do them.

auto-merge was automatically disabled November 5, 2025 03:55

Pull request was closed

@NguyenHoangSon96 NguyenHoangSon96 mentioned this pull request Nov 5, 2025
5 tasks
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.

[2.x] Influx generating invalid/broken config for vCenter

3 participants