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

Upgrade to Python 3.11 #14257

Closed
wants to merge 24 commits into from
Closed

Conversation

jessicamack
Copy link
Member

SUMMARY

Upgrade the default version of Python used to 3.11

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API
  • CLI
AWX VERSION
awx: 22.5.1.dev10+g694d190e57
ADDITIONAL INFORMATION

@@ -2,7 +2,7 @@

from django.conf import settings
from django.utils.translation import gettext_lazy as _
from thycotic.secrets.vault import SecretsVault
from delinea.secrets.vault import SecretsVault
Copy link
Member

@AlanCoding AlanCoding Jul 25, 2023

Choose a reason for hiding this comment

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

Also see #14207, it looks like we have 2 credential plugins that import this library. And multiple open PRs doing some fraction of the upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should rebase and merge the maintainers PR and then I can rebase and include that work.

Copy link
Member

Choose a reason for hiding this comment

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

That's good with me, I asked the question in that PR, because we should strive to keep all plugins functional.

Copy link
Member

Choose a reason for hiding this comment

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

I had additional conversation in that PR #14207 and left it with a minor request. I'm completely happy to get that in on its own. Knowing that, it would seem appropriate to remove that change from this python upgrade PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind removing it though I think it might cause errors because the lower versions of python-tss-sdk and python-dsv-sdk don't work with 3.11. Do we want to just ignore the errors for the moment since the other PR will be in shortly and address them?

Copy link
Member

Choose a reason for hiding this comment

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

Errors installing them (which breaks all builds), errors when using the plugin, or more subtle hypothetical python version compatibility issues which might not affect us anyway?

The PR #14207 has some clear steps forward, but they have not been responded too. Additionally, upgrading python-dsv-sdk is not attempted by that. Manually testing imports, I have found that upgrading python-dsv-sdk looks fairly straightforward and easy. In both cases, upgrading the library changes the import path.

If those changes really are needed, you could consider taking over that patch from them, including it in here, or doing a quick separate PR where you upgrade those 2 libraries. We have somewhat of a problem that we have a fragile testing situation, for which I have some potential work up at AlanCoding#81. If you want to verify functionality of these plugins after upgrade, you could try the manual instructions at tools/docker-compose/README.md which have been tested quite recently.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Aug 1, 2023
Comment on lines +113 to +119
serializer_class = mocker.patch('awx.api.serializers.InventoryUpdateDetailSerializer')
serializer = serializer_class.return_value
serializer.to_representation.return_value = {}

view = InventoryInventorySourcesUpdate()
response = view.post(mock_request)
assert response.data == expected
view = InventoryInventorySourcesUpdate()
response = view.post(mock_request)
assert response.data == expected
Copy link
Member

Choose a reason for hiding this comment

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

@jessicamack why was this changed

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests were throwing an error and this warning about using mocks as a context manager https://pytest-mock.readthedocs.io/en/latest/remarks.html#usage-as-context-manager so I reformatted the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

A long shot, but could this have been related to the test that we eventually disabled? Can you try reverting these changes and then run tests a) with that naughty test disabled and b) with the naughty test enabled? If we can get past tests with this change reverted, and maybe even get past the other test with this change reverted, then we should revert it.

@TheRealHaoLiu TheRealHaoLiu self-assigned this Aug 2, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

I might not be synced, but we need to make sure we regen/clean-up the licenses that were changed that we went back on, right? Like we regenerated without the full updates so we should have way fewer license changes now iiuc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like maybe git checkout devel -- $allthoselicensefiles and then regen and then git add $newlicensefiles && git commit, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

bump. @jessicamack Do you have a local copy where you fixed all the license files, etc? That seems to be one of the last pieces to at least mark this as complete. That and @relrod feedback. If you want, I can try to do the cleanup.

Copy link
Contributor

@jjwatt jjwatt left a comment

Choose a reason for hiding this comment

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

Please take a look at the code you reformatted early on and at the license files (if you haven't already).

awx/settings/defaults.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked component:api component:cli dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants