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

Remove Python 2 datetime compat fallbacks #81874

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 3, 2023

SUMMARY

$sbj.

ISSUE TYPE
  • Maintenance Pull Request
ADDITIONAL INFORMATION

N/A

@ansibot ansibot added needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 3, 2023
@ansibot

This comment was marked as outdated.

@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from b64732e to 4d16dc3 Compare October 3, 2023 21:54
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 4, 2023
@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from 4d16dc3 to a36fdaa Compare October 9, 2023 19:49
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 9, 2023
@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from a36fdaa to 4875507 Compare October 9, 2023 23:02
@ansibot

This comment was marked as resolved.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 9, 2023
@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from 4875507 to f6f8075 Compare October 10, 2023 13:39
@webknjaz
Copy link
Member Author

The test ansible-test sanity --test no-get-exception [explain] failed with 1 error:

lib/ansible/module_utils/compat/datetime.py:31:10: do not use `get_exception`

@mattclay apparently that no-get-exception test matches get_exception() within docstrings. Is it supposed to do that?

@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from f6f8075 to 4c9b5e5 Compare October 10, 2023 14:02
@ansibot

This comment was marked as resolved.

@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from 0b5fcdb to b6c3f8d Compare October 10, 2023 15:48
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Oct 10, 2023
@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from b6c3f8d to 886af13 Compare October 10, 2023 20:20
@webknjaz
Copy link
Member Author

So the CI failure is

01:24 _____________________ test_deprecate_without_list[stdin0] ______________________
01:24 [gw0] linux -- Python 3.12.0 /usr/bin/python3.12
01:24 
01:24 am = <ansible.module_utils.basic.AnsibleModule object at 0x7fca11c53530>
01:24 capfd = <_pytest.capture.CaptureFixture object at 0x7fca11c2dfd0>
01:24 
01:24     @pytest.mark.parametrize('stdin', [{}], indirect=['stdin'])
01:24     def test_deprecate_without_list(am, capfd):
01:24         with pytest.raises(SystemExit):
01:24             am.exit_json(deprecations='Simple deprecation warning')
01:24     
01:24         out, err = capfd.readouterr()
01:24         output = json.loads(out)
01:24 E         
01:24 E         ...Full output truncated (21 lines hidden), use '-vv' to show
01:24 
01:24 test/units/module_utils/basic/test_deprecate_warn.py:67: AssertionError

Which is weird because I cannon reproduce it locally with --docker.

@webknjaz
Copy link
Member Author

Aha.. So while :; do if ! bin/ansible-test units --docker --python 3.12 -vvvvvv --debug -- test/units/module_utils/basic/test_deprecate_warn.py; then break; fi; done doesn't reproduce the problem, but running much more tests does, like this:

$ while :; do if ! bin/ansible-test units --docker --python 3.12 -vvvvvv --debug -- test/units/module_utils/; then break; fi; done

@webknjaz
Copy link
Member Author

Reduced the repro to

$ while :; do if ! bin/ansible-test units --docker --python 3.12 -vvvvvv --debug -- test/units/module_utils/basic/test_deprecate_warn.py test/units/module_utils/compat/test_datetime.py; then break; fi; done

@webknjaz
Copy link
Member Author

Apparently, test/units/module_utils/compat/test_datetime.py imports the deprecated objects from the compat module, accessing them at import time, which triggers deprecation warnings that leak into stdout later on.

@webknjaz webknjaz force-pushed the maintenance/drop-datetime-py2-compat branch from 886af13 to 16354c1 Compare October 11, 2023 18:00
@webknjaz
Copy link
Member Author

@mattclay should we only deprecate the UTC object in this module?

Copy link
Member

@mattclay mattclay left a comment

Choose a reason for hiding this comment

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

It's too early to deprecate this, since collections may need it for compatibility across multiple non-EOL versions of ansible-core. Instead of deprecating it, add a deprecated comment (detected by pylint) so we can defer deprecation until 2.16 (the first version with this module_util) is the oldest non-EOL version.

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Oct 18, 2023
@webknjaz
Copy link
Member Author

@mattclay so I wasn't sure about deprecating since most of these aren't exact implementations of what's in stdlib.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 25, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants