change: Drop Python 3.5 support by lahirumaramba · Pull Request #542 · firebase/firebase-admin-python

change: Drop Python 3.5 support by lahirumaramba · Pull Request #542 · firebase/firebase-admin-python

Drop Python 3.5 support (Python 3.5 has reached EoL)
Resolves EOL for Python 3.5 #522

RELEASE NOTE: Dropped support for Python 3.5. Developers should to use Python 3.6 or higher when deploying the Admin SDK.

lahirumaramba changed the title ~~chore: Drop Python 3.5 support~~ change: Drop Python 3.5 support

Choose a reason for hiding this comment

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

Looks good. I only had one suggestion.

Have you thought about how to release/version this? In the past we have bumped major version numbers when we discontinued support for a platform version. But GCP is following a more relaxed policy with respect to that (albeit unofficially).

		# Fallback for Python 3.5
		from _pytest.monkeypatch import MonkeyPatch
		return MonkeyPatch()
		return MonkeyPatch()

Choose a reason for hiding this comment

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

Why not just return pytest.MonkeyPatch()?

Choose a reason for hiding this comment

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

Looks like pytest.MonkeyPatch() works for pytest 6.2 and up. Our CIs use the latest version so I think it would be safe to update this code.

Choose a reason for hiding this comment

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

Updated!

Choose a reason for hiding this comment

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

Should we just set pytest version to 6.2 and up in requirements.txt file?

Choose a reason for hiding this comment

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

Good idea! Updated.

Looks good. I only had one suggestion.

Have you thought about how to release/version this? In the past we have bumped major version numbers when we discontinued support for a platform version. But GCP is following a more relaxed policy with respect to that (albeit unofficially).

Thanks! I would prefer we do a major version bump to be consistent with our previous releases. Having said that, if we would rather align our releases with GCP policies a minor version bump would not be a big issue.

Choose a reason for hiding this comment

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

LGTM 👍

My personal preference is to bump the major version as well. GCP policy on this issue is still a work in progress, and might take a while to materialize. In the meantime, I'd be ok with bumping the major version for this change.

LGTM 👍

My personal preference is to bump the major version as well. GCP policy on this issue is still a work in progress, and might take a while to materialize. In the meantime, I'd be ok with bumping the major version for this change.

Sounds good! Let's do a major version bump for this then. Thanks!