Skip to content

Conversation

@ABrain7710
Copy link
Contributor

Description

  • Remove redis connection from top level so it isn't automatically created on import

This PR fixes #
#3047

Signed commits

  • Yes, I signed my commits.

@ABrain7710 ABrain7710 requested a review from sgoggins as a code owner July 8, 2025 02:42
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

pylint

tests/test_tasks/test_task_utlities/test_redis/test_redis_list.py|64 col 27| W0621: Redefining name 'redis_list' from outer scope (line 8) (redefined-outer-name)
tests/test_tasks/test_task_utlities/test_redis/test_redis_list.py|70 col 4| C0200: Consider using enumerate instead of iterating with range and len (consider-using-enumerate)

Ulincsys
Ulincsys previously approved these changes Jul 8, 2025
Copy link
Contributor

@Ulincsys Ulincsys left a comment

Choose a reason for hiding this comment

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

LGTM

@sgoggins
Copy link
Member

sgoggins commented Jul 8, 2025

@JohnStrunk / @MoralCode : The end to end test is timing out, but not "failing" in the ordinary sense. . I am wonder if the timeout is a parameter we need to set higher?

@MoralCode
Copy link
Contributor

@JohnStrunk / @MoralCode : The end to end test is timing out, but not "failing" in the ordinary sense. . I am wonder if the timeout is a parameter we need to set higher?

This isnt a timeout issue, the code in this PR made a change that broke something. The stacktrace above the timeout error points to:

augur-1 | File "/augur/keyman/KeyClient.py", line 162, in __init__
augur-1 | self.stdin: PubSub = self.conn.pubsub(ignore_subscribe_messages = True)
augur-1 | ^^^^^^^^^
augur-1 | AttributeError: 'KeyPublisher' object has no attribute 'conn'

This was also flagged by a bot that is installed https://github.com/chaoss/augur/pull/3218/files#r2191362618 (among many other warnings)

Copy link
Contributor

@MoralCode MoralCode left a comment

Choose a reason for hiding this comment

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

I'd recommend holding off until the various warnings and errors caught by the automation tools (the bot and the E2E CI job) are resolved


self.id = getpid()
self.stdin: PubSub = conn.pubsub(ignore_subscribe_messages = True)
self.stdin: PubSub = self.conn.pubsub(ignore_subscribe_messages = True)
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be fixed.

Update to address error message in end to end build. 

Signed-off-by: Sean P. Goggins <outdoors@acm.org>
Copy link
Member

@sgoggins sgoggins left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@Ulincsys Ulincsys left a comment

Choose a reason for hiding this comment

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

LGMT

@sgoggins sgoggins merged commit 5567bbd into main Jul 8, 2025
14 checks passed
@sgoggins sgoggins deleted the redis-conn branch July 26, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants