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

Put back deprecation warnings #63315

Merged
merged 8 commits into from
Jan 10, 2023

Conversation

waynew
Copy link
Contributor

@waynew waynew commented Dec 13, 2022

What does this PR do?

Fixes #62185 - now we emit DeprecationWarnings again.

Can test against the MCVE in #59917

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@waynew waynew requested a review from a team as a code owner December 13, 2022 19:26
@waynew waynew requested review from Ch3LL and removed request for a team December 13, 2022 19:26
s0undt3ch
s0undt3ch previously approved these changes Dec 13, 2022
Ch3LL
Ch3LL previously approved these changes Dec 13, 2022
@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Dec 13, 2022
@max-arnold
Copy link
Contributor

max-arnold commented Dec 14, 2022

@waynew Thanks for tracking that down!

Does it make sense to add a unit test to check that deprecation warnings actually work? IMO this feature is important enough to warrant a dedicated test case. The regression was left undetected for a couple of years!

@waynew waynew added this to the Sulphur v3006.0 milestone Dec 16, 2022
@waynew waynew dismissed stale reviews from Ch3LL and s0undt3ch via 1bc9527 December 20, 2022 05:18
@waynew waynew force-pushed the fix/warnings/restore-deprecation branch from f24a478 to 1bc9527 Compare December 20, 2022 05:18
Checked the
https://docs.python.org/3/library/warnings.html#warnings.filterwarnings
docs but there wasn't any obvious reason why our warnings disappear.
This restores the DeprecationWarnings showing up on the command line.
Rather than just rely on the test suite, we also add a deprecation
warning to the test module, which will enable a simple method for
ensuring that DeprecationWarnings are correctly emitted.
@s0undt3ch s0undt3ch force-pushed the fix/warnings/restore-deprecation branch from 1bc9527 to 9c896fe Compare December 20, 2022 09:59
Ch3LL
Ch3LL previously approved these changes Dec 21, 2022
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

Test failures look valid.

@waynew waynew force-pushed the fix/warnings/restore-deprecation branch 6 times, most recently from 7ec752c to ee0accb Compare January 10, 2023 01:30
Due to some of the versions of dependencies we're using, these tests may
emit some specific errors.
@waynew waynew force-pushed the fix/warnings/restore-deprecation branch from ee0accb to 0d48439 Compare January 10, 2023 02:40
@waynew
Copy link
Contributor Author

waynew commented Jan 10, 2023

Last failures look like flaky bits

@waynew
Copy link
Contributor Author

waynew commented Jan 10, 2023

@s0undt3ch test failures look to be sorted, finally 🙃

@Ch3LL Ch3LL merged commit 3f2e32a into saltstack:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] salt.utils.versions.warn_until doesn't seem to be warn-ing
5 participants