sbp commented on code in PR #337:
URL:
https://github.com/apache/tooling-trusted-releases/pull/337#discussion_r2550418371
##########
atr/svn/pubsub.py:
##########
@@ -54,24 +55,49 @@ async def start(self) -> None:
"""Run forever, processing PubSub payloads as they arrive."""
# TODO: Add reconnection logic here?
# Or does asfpy.pubsub.listen() already do this?
- log.info("SVNListener.start() called")
- async for payload in asfpy.pubsub.listen(
- # TODO: Upstream this change to BAT
- urllib.parse.urljoin(self.url, self.topics),
- username=self.username,
- password=self.password,
- ):
- if (payload is None) or ("stillalive" in payload):
- continue
+ if not self.url:
+ log.error("PubSub URL is not configured")
+ log.warning("SVNListener disabled: no URL provided")
+ return
- pubsub_path = str(payload.get("pubsub_path", ""))
- if not pubsub_path.startswith(_WATCHED_PREFIXES):
- # Ignore commits outside dist/dev or dist/release
- continue
+ if not self.username or not self.password:
Review Comment:
Please parenthesize subexpressions, as per code conventions.
##########
atr/svn/pubsub.py:
##########
@@ -54,24 +55,49 @@ async def start(self) -> None:
"""Run forever, processing PubSub payloads as they arrive."""
# TODO: Add reconnection logic here?
# Or does asfpy.pubsub.listen() already do this?
- log.info("SVNListener.start() called")
- async for payload in asfpy.pubsub.listen(
- # TODO: Upstream this change to BAT
- urllib.parse.urljoin(self.url, self.topics),
- username=self.username,
- password=self.password,
- ):
- if (payload is None) or ("stillalive" in payload):
- continue
+ if not self.url:
+ log.error("PubSub URL is not configured")
+ log.warning("SVNListener disabled: no URL provided")
+ return
- pubsub_path = str(payload.get("pubsub_path", ""))
- if not pubsub_path.startswith(_WATCHED_PREFIXES):
- # Ignore commits outside dist/dev or dist/release
- continue
+ if not self.username or not self.password:
+ log.error("PubSub credentials not configured")
+ log.warning("SVNListener disabled: missing credentials")
+ return
+
+ if not self.url.startswith(("http://";, "https://";)):
+ log.error(
+ "Invalid PubSub URL: %r. Expected full URL like
'https://pubsub.apache.org:2069'",
+ self.url,
+ )
+ log.warning("SVNListener disabled due to invalid URL")
+ return
+
+ full_url = urllib.parse.urljoin(self.url, self.topics)
+ log.info("SVNListener starting with URL: %s", full_url)
Review Comment:
I think we're always using f-strings for log statements, and indeed anything
where printf style could otherwise be accepted. We should probably add a code
convention about this. Please change to f-string style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
