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

Enforce that all scripts use Python 3 rather than Python 2 #43

Merged
merged 16 commits into from
May 12, 2023

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Jun 15, 2022

πŸ—£ Description

This pull request makes a change to enforce that all the scripts contained in this repo use Python 3 vice Python 2.

πŸ’­ Motivation and context

The one remaining script that used Python 2 code appears to work with either Python 2 or Python 3, so it makes sense to insist on Python 3 now that Python 2 is EOL.

πŸ§ͺ Testing

All automated tests pass.

@mcdonnnj, is this one line change something that can be manually done in production to verify that it functions as expected?

βœ… Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
  • All new and existing tests pass.

βœ… Pre-merge checklist

  • Finalize version.

βœ… Post-merge checklist

  • Create a release.

@jsf9k jsf9k added bug This issue or pull request addresses broken functionality improvement This issue or pull request will add new or improve existing functionality labels Jun 15, 2022
@jsf9k jsf9k self-assigned this Jun 15, 2022
@jsf9k jsf9k force-pushed the improvement/insist-on-python3 branch from 7f95b78 to 0e0aaed Compare June 15, 2022 20:30
@dav3r
Copy link
Member

dav3r commented Jun 16, 2022

@mcdonnnj, is this one line change something that can be manually done in production to verify that it functions as expected?

We should be able to test that in either @mcdonnnj or my CyHy dev environments.

@jsf9k
Copy link
Member Author

jsf9k commented Jul 5, 2022

FYI, the main branch of this project is now failing APB builds.

@jsf9k jsf9k marked this pull request as ready for review September 9, 2022 15:06
@dav3r dav3r added this to the Summer CyHy AMI Spectacular milestone Sep 20, 2022
@jsf9k jsf9k force-pushed the improvement/insist-on-python3 branch from 0e0aaed to 14cbeb9 Compare September 29, 2022 21:16
Realign setup.py with the version used in
cisagov/skeleton-python-library now that we no longer need to support
Python 2.
We no longer need to adjust the import depending on which major Python
version is being used.
Until this project is converted into a proper Python package this is
how it needs to be updated to reflect the package version.
Make sure the `build.yml` workflow in this project mirrors the one in
cisagov/skeleton-python-library now that we no longer need to support
Python 2.
@jsf9k
Copy link
Member Author

jsf9k commented Apr 18, 2023

@mcdonnnj, could you leave a comment here explaining why this PR cannot yet be merged?

@mcdonnnj
Copy link
Member

mcdonnnj commented Apr 18, 2023

@jsf9k Until we're deploying Debian Buster/Bullseye instances we have to consider Debian Stretch which only has Python 3.5. The lowest version of Python 3 this supports is 3.6 per

python_requires=">=3.6",
This PR can hopefully be merged this week as I finalize testing and prepare to deploy Debian Buster/Bullseye instances.

@jsf9k
Copy link
Member Author

jsf9k commented Apr 26, 2023

@jsf9k Until we're deploying Debian Buster/Bullseye instances we have to consider Debian Stretch which only has Python 3.5. The lowest version of Python 3 this supports is 3.6 per

python_requires=">=3.6",

This PR can hopefully be merged this week as I finalize testing and prepare to deploy Debian Buster/Bullseye instances.

Now that Debian Stretch is dead as Dillinger, does this change anything?

@mcdonnnj
Copy link
Member

mcdonnnj commented Apr 26, 2023

@jsf9k Until we're deploying Debian Buster/Bullseye instances we have to consider Debian Stretch which only has Python 3.5. The lowest version of Python 3 this supports is 3.6 per

python_requires=">=3.6",

This PR can hopefully be merged this week as I finalize testing and prepare to deploy Debian Buster/Bullseye instances.

Now that Debian Stretch is dead as Dillinger, does this change anything?

No. If there is a critical problem with the Buster images I can always create a cursed Ansible role to pull from the Debian archive for package updates to fall back on Stretch. I would rather minimize any peripheral impact until I get new images built, deployed, and verified.

Warning
I realized belatedly that this PR was never included in my batch of testing. I have a branch in cisagov/ansible-role-cyhy-feeds and am troubleshooting installing this package and its dependencies under Python 3.

Pull in the current pre-commit configuration from
cisagov/skeleton-python-library and update the mypy hook dependencies.
This is necessary to restore a functional pre-commit configuration to
the project.
This only resulted in the `black` hook adjusting a single file.
In Python 3 files are by default opened in text-mode, which suites the
JSON we are outputting, and file seeking behavior is different. This
change is necessary to get compatible behavior now that this project
only supports Python 3.
As currently written the following warning is emitted when using the
scripts:
DeprecationWarning: The SafeConfigParser class has been renamed to ConfigParser in Python 3.2. This alias will be removed in future versions. Use ConfigParser directly instead.

Since this project will only support Python 3.6+ going forward it makes
sense to update to remove this warning.
Pull in the current version in cisagov/skeleton-python-library as that
is the most comparable to the current workflow definition. This is
necessary to fix testing Python 3.6.
@mcdonnnj
Copy link
Member

mcdonnnj commented May 9, 2023

@dav3r @jsf9k Would you mind reviewing this PR now that I've added a number of changes. I tested this branch (with my updates) in my development CyHy environment and was able to confirm that it worked without errors.

Copy link
Member Author

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

I can only comment on this PR, but if I could I would request a few minor changes.

.github/workflows/build.yml Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Aside from what @jsf9k noted, everything else looks fantastic to me.

@jsf9k Reminder that you (or any of us) still need to update the branch protection to remove the outdated checks.

jsf9k and others added 2 commits May 9, 2023 23:18
Co-authored-by: dav3r <david.redmin@trio.dhs.gov>
Update the package's classifiers to align with both the testing being
done and the current defaults in cisagov/skeleton-python-library.
Copy link
Member Author

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

I can only comment on this PR, but I would approve it with gusto if I could!

@mcdonnnj mcdonnnj merged commit 0d51f63 into develop May 12, 2023
49 checks passed
@mcdonnnj mcdonnnj deleted the improvement/insist-on-python3 branch May 12, 2023 19:03
@jsf9k
Copy link
Member Author

jsf9k commented May 12, 2023

πŸ₯³

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality improvement This issue or pull request will add new or improve existing functionality
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants