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.

Sign up for GitHub

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

Jump to bottom

Remove crypt support from ansible.utils.encrypt #81721

Merged
merged 3 commits into from
Jan 18, 2024
Merged

Remove crypt support from ansible.utils.encrypt #81721

merged 3 commits into from
Jan 18, 2024
+27 −227

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Sep 19, 2023

SUMMARY

Fixes #81717

ISSUE TYPE
  • Feature Pull Request

@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 19, 2023
@ansibot

This comment was marked as resolved.

Sign in to view
@webknjaz

This comment was marked as resolved.

Sign in to view
@azure-pipelines

This comment was marked as resolved.

Sign in to view
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Sep 19, 2023
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Sep 19, 2023
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. and removed ci_verified Changes made in this PR are causing tests to fail. labels Sep 20, 2023
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Sep 20, 2023
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 22, 2023
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Sep 25, 2023
@mkrizek mkrizek force-pushed the issue-81717 branch 2 times, most recently from 29c7e0e to 3ec9fa5 Compare September 27, 2023 11:52
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 27, 2023
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Sep 27, 2023
@mkrizek mkrizek force-pushed the issue-81717 branch 2 times, most recently from 8dfff34 to 8b51586 Compare September 29, 2023 15:06
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 29, 2023
@mkrizek mkrizek force-pushed the issue-81717 branch 2 times, most recently from 2d597c5 to 618aa34 Compare October 3, 2023 14:44
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 3, 2023
mkrizek
mkrizek commented Oct 3, 2023
View reviewed changes
test/integration/targets/become/aliases Outdated
@@ -2,3 +2,4 @@ destructive
shippable/posix/group1
context/target
gather_facts/no
setup/always/setup_passlib_controller
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattclay I added this for every target that uses setup_test_user. I feel like setup_passlib_controller should be run as a dependency/part of setup_test_user but I couldn't make it work for setup_passlib_controller to be run always on a controller side as setup_test_user is used either as a meta dependency of targets or as needs/target/setup_test_user and called explicitly in a test either in roles: or via include_role.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, currently setup/always/ doesn't work recursively. It's detected as a dependency, but it's not actually invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note that it's required by setup_test_user, at least.

@mkrizek mkrizek requested a review from mattclay October 3, 2023 18:08
mattclay
mattclay reviewed Oct 4, 2023
View reviewed changes
test/integration/targets/setup_passlib_controller/runme.sh

# Requirements have to be installed prior to running ansible-playbook
# because plugins and requirements are loaded before the task runs
python -m pip install passlib
Copy link
Member

Choose a reason for hiding this comment

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

This isn't using a virtual environment, which is going to be problematic in the not-too-distant future. It's probably time for me to add support for ansible-test managed virtual environments for integration tests, which would greatly simplify this and avoid the issue of working without a virtual environment.

@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 11, 2023
@mkrizek
Copy link
Contributor Author

mkrizek commented Nov 30, 2023

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot removed 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 Nov 30, 2023
@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 Dec 14, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 9, 2024
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 17, 2024
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jan 17, 2024
@mkrizek @mattclay
Update test/integration/targets/setup_passlib_controller/runme.sh
f3dc152 
Co-authored-by: Matt Clay <matt@mystile.com>
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 17, 2024
@mkrizek mkrizek requested a review from mattclay January 17, 2024 20:15
@ansibot ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jan 17, 2024
@mkrizek mkrizek merged commit 3f74bc0 into ansible:devel Jan 18, 2024
62 checks passed
@mkrizek mkrizek deleted the issue-81717 branch January 18, 2024 07:23
webknjaz
webknjaz reviewed Jan 19, 2024
View reviewed changes
test/units/utils/test_encrypt.py
@@ -35,31 +21,12 @@ def assert_hash(expected, secret, algorithm, **settings):
assert excinfo.value.args[0] == "passlib must be installed and usable to hash with '%s'" % algorithm
Copy link
Member

Choose a reason for hiding this comment

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

@mkrizek the entire else-branch here lost coverage with the changes in this PR. Should the assertion here be made unconditional or new tests added?

Copy link
Member

Choose a reason for hiding this comment

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

See https://app.codecov.io/gh/ansible/ansible/commit/6935c8e303440addd3871ecf8e04bde61080b032/indirect-changes#3ab5fa106f6a24247d92fd39c8e79a4f-R19

Copy link
Member

Choose a reason for hiding this comment

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

And this is the only place with dead code in this test module: https://app.codecov.io/gh/ansible/ansible/blob/devel/test%2Funits%2Futils%2Ftest_encrypt.py

@ansible ansible locked and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@webknjaz webknjaz webknjaz left review comments

@mattclay mattclay mattclay approved these changes

@bcoca bcoca bcoca approved these changes

Assignees
No one assigned
Labels
feature This issue/PR relates to a feature request. has_issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove crypt support from ansible.utils.encrypt for 2.17
5 participants
@mkrizek @ansibot @webknjaz @mattclay @bcoca