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

Loosen cassandra-driver requirement to allow latest version #15022

Merged
merged 1 commit into from Mar 29, 2021

Conversation

ashb
Copy link
Member

@ashb ashb commented Mar 25, 2021

I have left the CASS_ settings in the dockerfile -- they do nothing when a .whl is available, but will do something for Py3.9 for example.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@@ -72,20 +72,17 @@ def setUp(self):
hook.shutdown_cluster()

def test_get_conn(self):
with mock.patch.object(Cluster, "connect") as mock_connect, mock.patch(
"socket.getaddrinfo", return_value=[]
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 previous mocking didn't work anymore, but this was to some extend, testing the internals of Cassandra.

I have replaced it with just ensuring we call `Cluster() correctly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This likely no longer works because cython "caches"/looks up the built in getaddrinfo at compile time, so the mocking has no effect.

@github-actions github-actions bot removed the full tests needed We need to run full set of tests for this PR to merge label Mar 25, 2021
@ashb
Copy link
Member Author

ashb commented Mar 25, 2021

This was first pinned in #7194

@ashb ashb added the full tests needed We need to run full set of tests for this PR to merge label Mar 26, 2021
@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb
Copy link
Member Author

ashb commented Mar 26, 2021

@potiuk How do I make this rebuild with latest dependencies? Right now this is testing against the old cassandra-driver dep because of the Constraints. I thought there was some label I could apply, but I can't find it in the CI.rst or similar.

Am I making up such a feature?

Edit: Ah, yes I'm making this up. I see what the problem is though. #15033

@ashb ashb force-pushed the update-cass-latest-version branch from be1d6e9 to a044045 Compare March 26, 2021 17:07
@ashb
Copy link
Member Author

ashb commented Mar 26, 2021

Cool, now actually running with 3.25.0

@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

1 similar comment
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

@potiuk
Copy link
Member

potiuk commented Mar 26, 2021

Just a word of warning here - this might become another blocker if we finally manage to make Python 3.9 working. From what I see there is no 3.9 wheel (and even the driver itself does not seem to support 3.9 version).

@ashb
Copy link
Member Author

ashb commented Mar 26, 2021

Oh good point.

@ashb
Copy link
Member Author

ashb commented Mar 26, 2021

Looks like 3.9 support at all may be questionable for Cassandra https://datastax-oss.atlassian.net/browse/PYTHON-1246

There is no explicit python_requires in their dist, but they do only have the 3.8 classifier.

@potiuk
Copy link
Member

potiuk commented Mar 26, 2021

Looks like 3.9 support at all may be questionable for Cassandra https://datastax-oss.atlassian.net/browse/PYTHON-1246

This is not a full-blocker in general. I'd love if we could get rid of those variables.

Maybe that is a good time to add the feature to make some providers non-compatible with some versions of python (3.9 for sure). We could make a pytest exception for relevant test and conditional Docker image build to exclude some providers when the image for 3.9 is built. This seems the only viable option to get 3.9 quickly and get the #11950 done.

WDYT?

@ashb ashb force-pushed the update-cass-latest-version branch from a044045 to bdfdacb Compare March 26, 2021 21:31
@ashb
Copy link
Member Author

ashb commented Mar 26, 2021

Can do yes.

In this case I have left the CASS_ settings in the dockerfile -- they do nothing when a whl is available, but will do something for Py3.9 for example.

@potiuk
Copy link
Member

potiuk commented Mar 26, 2021

Added a #15045 for excluding some providers from 3.9

@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

For py 3.6-3.8 we don't need the CASS_ settings in the Dockerfiles but
as Datastax haven't yet published a Python 3.9 wheel, I have kept the
settings for now -- they are ignored when installing a wheel, but will
be used if the python version (i.e. 3.9) doesn't have a published wheel.
@ashb ashb force-pushed the update-cass-latest-version branch from bdfdacb to 4b7b091 Compare March 29, 2021 09:40
@ashb ashb merged commit a7a558e into apache:master Mar 29, 2021
@ashb ashb deleted the update-cass-latest-version branch March 29, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:providers full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants