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: Add method to inform of deprecated plugin option values #11987

Merged
merged 4 commits into from
Nov 28, 2022

Conversation

Hipska
Copy link
Contributor

@Hipska Hipska commented Oct 12, 2022

Required for all PRs

This will make it possible for plugin authors to deprecate option values they do not support/use anymore.

At the same time, the docs for deprecating options is updated a bit to also include sample for unused options.

@Hipska Hipska requested a review from srebhan October 12, 2022 07:45
@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 Oct 12, 2022
@srebhan
Copy link
Contributor

srebhan commented Oct 31, 2022

Hmmm why do we want another function? You could do this using models.PrintOptionDeprecationNotice() and the corresponding text, e.g.

models.PrintOptionDeprecationNotice(telegraf.Warn, "category.pluginname", "optionname", telegraf.DeprecationInfo{
				Since:     "1.24.3",
				RemovalIn: "2.0.0",
				Notice:    "value 'whatever' deprecated",
			})

@Hipska
Copy link
Contributor Author

Hipska commented Oct 31, 2022

Clearer error message, as your suggestion will say that optionname itself is deprecated..

@Hipska
Copy link
Contributor Author

Hipska commented Nov 7, 2022

What do you think @srebhan?

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Some minor comments @Hipska...

models/log.go Outdated Show resolved Hide resolved
models/log_test.go Outdated Show resolved Hide resolved
models/log_test.go Outdated Show resolved Hide resolved
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@Hipska Hipska force-pushed the feat/deprecate_option_values branch from 81dc11d to f4ef4b7 Compare November 10, 2022 13:09
@Hipska
Copy link
Contributor Author

Hipska commented Nov 10, 2022

!retry-checks

@Hipska
Copy link
Contributor Author

Hipska commented Nov 10, 2022

!retry-failed

@Hipska Hipska requested a review from srebhan November 10, 2022 16:08
@powersj
Copy link
Contributor

powersj commented Nov 10, 2022

!retry-failed

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -1.42 % for linux amd64 (new size: 154.9 MB, nightly size 157.1 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz
static_linux_amd64.tar.gz

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Added require.Eventually example.

models/log_test.go Outdated Show resolved Hide resolved
@Hipska Hipska requested a review from srebhan November 22, 2022 14:00
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Beautiful. Thanks for your effort @Hipska!

May I ask you to please convert the existing tests in that file to the same schema in a separate PR to serve as a good example!? :-)

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 25, 2022
@Hipska
Copy link
Contributor Author

Hipska commented Nov 25, 2022

Okay, can you create an Issue for that and assign to me?

@Hipska
Copy link
Contributor Author

Hipska commented Nov 25, 2022

This feature would be used first to deprecate the netsnmp translator.

@powersj powersj merged commit 63c8a78 into influxdata:master Nov 28, 2022
@Hipska Hipska deleted the feat/deprecate_option_values branch December 8, 2022 13:28
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 ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants