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

[CIVP-24658] REF Remove PubNub #425

Merged
merged 5 commits into from
Nov 1, 2021
Merged

Conversation

jacksonlee-civis
Copy link
Member

@jacksonlee-civis jacksonlee-civis commented Oct 29, 2021

This PR removes all PubNub-related code, as we've stopped using PubNub for quite a while. There's no effect to the users of civis-python, because the code being removed has not been triggered since PubNub usage was dropped from Civis Platform.

# Otherwise the job might complete after polling and before
# we subscribe, causing us to miss the notification.
with self._condition:
if hasattr(self.client, 'channels') and not self.subscribed:
Copy link
Member Author

Choose a reason for hiding this comment

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

All the PubNub code was triggered by this sort of "if there's the Channels endpoint and not yet subscribed, do the PubNub thing" logic. Since the Channels endpoint has been removed, code removal in this PR is practically about dropping all references to PubNub as well as related references of "subscribe", "listen", and "channels".

Copy link
Contributor

@skomanduri skomanduri left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for tackling this and tracking down so much code.

I still see some references to pubnub/channels in cassettes and in MANIFEST.in. Are you able to remove those lines/files as well? Since you've removed the test code, I think you should be able to remove all the cassettes that are calling https://api.civisanalytics.com/channels.

civis/futures.py Outdated Show resolved Hide resolved
@jacksonlee-civis
Copy link
Member Author

@skomanduri Thanks for catching more PubNub/Channels remnants! I've just removed those as well and the CI builds have passed. This PR is ready for another review.

Copy link
Contributor

@skomanduri skomanduri left a comment

Choose a reason for hiding this comment

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

LGTM

🎉 🎉 🎉

@jacksonlee-civis jacksonlee-civis merged commit 04081c4 into master Nov 1, 2021
@jacksonlee-civis jacksonlee-civis deleted the civp-24658-remove-pubnub branch November 1, 2021 17:03
jacksonlee-civis added a commit that referenced this pull request Nov 9, 2021
…re (#426)

* ENH add job and run IDs to CivisJobFailure

* MAINT update changelog

* ENH move #416 changes from ContainerFuture to CivisFuture

* FIX adjust ContainerFuture and tests

* ENH log retrieval works for CivisFuture more generally

* RM _poll_and_set_api_result (should've been rm-ed at #425)

* MAINT update changelog

* TST make ModelFuture tests work with client.jobs.list_runs_logs

* TST update mock client for ModelFuture tests

* TST more clean-up for ModelFuture tests

* FIX comment

* ENH rewrite logic to add job/run ID in CivisJobFailure

* TST refactor _create_poller_mock

* TST poller call count for poll_on_creation True/False

* ENH more natural log message order

* ENH wait for delayed traceback in log

* ENH clearer error message

* ENH simplify error message a bit

* TST revert unnecessary tweak
@jacksonlee-civis jacksonlee-civis added this to the v1.16.0 milestone Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants