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

Drop boto 2 support #731

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Drop boto 2 support #731

merged 1 commit into from
Jul 31, 2023

Conversation

jairhenrique
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Merging #731 (b487695) into master (4f70152) will increase coverage by 0.94%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   89.94%   90.88%   +0.94%     
==========================================
  Files          28       27       -1     
  Lines        1790     1778      -12     
  Branches      254      321      +67     
==========================================
+ Hits         1610     1616       +6     
+ Misses        145      127      -18     
  Partials       35       35              
Impacted Files Coverage Δ
vcr/patch.py 89.56% <ø> (+1.13%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

Hi @jairhenrique, my impression is that

  • boto 2's last release is from July 2018 based on https://pypi.org/project/boto/ which is 5 years ago
  • getting this code removed seems healthy
  • and the very way done here also,
  • but it is also a break in backwards compatibility.

I would recommend that we (1) bump the version to 6.0.0 when dropping this code also and that (2) we check back with @kevin1024 if this drop and another major version bump is tolerable in his book also. Personally I think won't cause much damage if at all.

@jairhenrique
Copy link
Collaborator Author

@hartwork I didn't investigate to find out since when the tests don't run with boto. This looks more like a code cleanup to me, rather than a support removal, because this has already been done.

I don't think a new major version is needed.

@hartwork
Copy link
Collaborator

rather than a support removal, because this has already been done.

@jairhenrique are we sure that support for boto 2 is currently unusable? If not, I'd consider it removal of a feature and hence a breaking change.

@vEpiphyte
Copy link

vEpiphyte commented Jun 28, 2023

The trove classifiers for python support of boto2 and vcrpy only have python 3 listed as an overlapping entry. The last boto2 release declares python 3.5 as its highest supported version; whereas vcrpy has a >3.8 python requirement. It could be argued that attempting to keep supporting boto2 is a uphill battle on that conflicting information alone.

EDIT:
Boto2's highest supported version is 3.4., not 3.5.

@hartwork
Copy link
Collaborator

hartwork commented Jun 28, 2023

I'm with the two of you that boto 2 support should be dropped. The question if it's a backwards compatible change is tied to the question whether it's possible to use boto 2 with recent VCR.py. boto 2 does not prohibit use with Python >=3.8 in setup.py. The boto tests required an AWS account to run. Does one of you have an AWS account that could be used to run tests/integration/test_boto.py against to get a clear answer on that?

@vEpiphyte
Copy link

@hartwork I may be able to try that this afternoon.

@vEpiphyte
Copy link

@hartwork @jairhenrique
Some tests work, some tests fail.

$ python -m pytest -s -v tests/integration/test_boto.py 
========================================================================================= test session starts =========================================================================================
platform linux -- Python 3.11.1, pytest-7.4.0, pluggy-1.0.0 -- /home/epiphyte/.pyenv/versions/3.11.1/envs/vcrpy3111/bin/python
cachedir: .pytest_cache
rootdir: /home/epiphyte/PycharmProjects/vcrpy
configfile: pyproject.toml
plugins: asyncio-0.21.0, httpbin-2.0.0, cov-4.1.0
asyncio: mode=Mode.STRICT
collected 5 items                                                                                                                                                                                     

tests/integration/test_boto.py::test_boto_stubs PASSED
tests/integration/test_boto.py::test_boto_without_vcr FAILED
tests/integration/test_boto.py::test_boto_medium_difficulty FAILED
tests/integration/test_boto.py::test_boto_hardcore_mode FAILED
tests/integration/test_boto.py::test_boto_iam PASSED

============================================================================================== FAILURES ===============================================================================================

The failing tests rely on a public bucket boto-demo-1394171994. I do not think this bucket exists anymore?

The test referring to it came from 6bb6756 which is 9 years old. @kevin1024 was this bucket removed?

@kevin1024
Copy link
Owner

IIRC (and it's been a long 9 years!) The bucket doesn't need to have anything in it for the tests to pass; it just needs to exist and the AWS creds need to have access to that bucket.

@jairhenrique
Copy link
Collaborator Author

I think that whether or not the tests pass with the boto, it doesn't matter. We can think of it here as Python, the VCR still works with Python 3.7, but we stopped supporting it because it's a version that reached EOL. With BOTO it's the same thing, it may work, but the library is no longer supported. Those who use old versions, install old versions of dependencies.

I believe the big question here is which number should we increment the VCR to, 5.1.0 or 6.0.0, and I'm not sure if it's necessary to go up to a 6.0.0 version since at other times, things have been deprecated around here and just incremented the MINOR semantic versioning number.

What is your opinion about this?

@hartwork
Copy link
Collaborator

@jairhenrique I consider these things interconnected. In my mind the two healthy options are:

  • a) Ignore whether boto support is functioning and do a major version bump (so that not even in theory working in-use support for boto can break for someone without a proper semver warning sign)
  • b) Prove boto support unusable, drop it, no major version bump

I'd be good with either of these, but would personally have trouble supporting a mixture. If the two of you — @jairhenrique and @kevin1024 — find something to agree upon in this context, I can be good with that though. I hope that's good enough.

@kevin1024
Copy link
Owner

  • a) Ignore whether boto support is functioning and do a major version bump (so that not even in theory working in-use support for boto can break for someone without a proper semver warning sign)

I vote for this one. No need to break support if it’s working, but also no need to put in the effort to make sure it’s supported. Let’s just document that support is dropped and do a major version bump.

@jairhenrique
Copy link
Collaborator Author

I'll merge this PR after #749

@jairhenrique jairhenrique merged commit 6446d00 into master Jul 31, 2023
7 checks passed
@jairhenrique jairhenrique deleted the drop-boto branch July 31, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants