-
-
Notifications
You must be signed in to change notification settings - Fork 38
{username}-afk_{host} #74
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
base: master
Are you sure you want to change the base?
{username}-afk_{host} #74
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 102465f in 2 minutes and 15 seconds
More details
- Looked at
45
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. aw_watcher_afk/afk.py:32
- Draft comment:
Using shell=True with a static command is acceptable, but ensure no future modifications allow untrusted input. Consider catching specific exceptions (e.g., subprocess.CalledProcessError) for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is about security best practices, but it's more speculative ("ensure no future modifications") than pointing out a current issue. The exception handling suggestion is a minor improvement but not critical since the function already has error handling that accomplishes its goal. The command is indeed static and safe as-is.
The suggestion about specific exception handling could make the code more maintainable. The security warning about shell=True is valid since it's a common source of vulnerabilities.
However, the current code is safe as implemented, and the comment is more about potential future issues than current problems. The existing error handling is sufficient for the simple use case.
The comment should be deleted as it's more speculative and advisory rather than pointing out a clear issue that needs fixing.
2. aw_watcher_afk/afk.py:58
- Draft comment:
Ensure the new bucket naming convention (using username) is consistent across deployments, especially when the username returns 'unknown'. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. aw_watcher_afk/afk.py:62
- Draft comment:
Bucket naming now relies on the username from get_logged_in_user. Ensure that falling back to 'unknown' won't cause naming collisions or other issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This comment appears to be asking the author to verify/ensure something rather than pointing out a clear issue. The fallback to 'unknown' seems like a reasonable default that wouldn't cause naming collisions since it would still be combined with the unique hostname. The comment doesn't identify any specific problem that needs fixing.
Maybe there could be a real issue with bucket naming collisions if multiple users get the 'unknown' fallback on the same host?
Even if multiple users get 'unknown', they would likely be running their own instances of the application with different ports/configurations, so this isn't a critical issue that needs addressing.
This comment should be deleted as it's asking for verification without identifying a clear problem that needs fixing.
4. aw_watcher_afk/afk.py:93
- Draft comment:
Minor typographical inconsistency: The log message at line 93 uses 'afkwatcher' while the class is named 'AFKWatcher'. Consider aligning the naming for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_sFjoWfJSU1BcuATc
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -27,6 +29,15 @@ | |||
logger = logging.getLogger(__name__) | |||
td1ms = timedelta(milliseconds=1) | |||
|
|||
def get_logged_in_user(): | |||
try: | |||
user = subprocess.check_output("who | awk '{print $1}' | head -n 1", shell=True).decode().strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using Python’s built-in getpass.getuser()
for retrieving the current user, which is more cross‑platform and avoids using shell=True
with subprocess
.
user = subprocess.check_output("who | awk '{print $1}' | head -n 1", shell=True).decode().strip() | |
user = getpass.getuser() |
Did you mean to submit this upstream? It will probably break the web UI, which expects the bucket name to start with aw-watcher-afk (iirc). Better to use a unique hostname per user (and ideally also a unique |
Important
Adds logged-in username to
bucketname
inAFKWatcher
by introducingget_logged_in_user()
inafk.py
.get_logged_in_user()
function inafk.py
to retrieve the current logged-in user using a shell command.AFKWatcher.__init__()
to include the logged-in username inbucketname
format as"{username}-afk_{host}"
.This description was created by
for 102465f. It will automatically update as commits are pushed.