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

pids: case insensitive string comparison for process names #52564

Merged
merged 5 commits into from Mar 14, 2019

Conversation

CodeAbyss
Copy link
Contributor

SUMMARY
"Fixes #52563"

ISSUE TYPE

  • Bugfix Pull Request

COMPONENT NAME
pids

ADDITIONAL INFORMATION
This fix will compare the user input process name and psutil returned process name with case insensitiveness. This will make the pids component more robust and usable.

@ansibot
Copy link
Contributor

ansibot commented Feb 19, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch support:community This issue/PR relates to code supported by the Ansible community. system System category labels Feb 19, 2019
@Akasurde

This comment has been minimized.

@Akasurde Akasurde self-requested a review February 19, 2019 11:22
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. needs_triage Needs a first human triage before being processed. labels Feb 19, 2019
Fixed "AttributeError: 'NoneType' object has no attribute 'lower'" error for "Empty" or None strings passed as process name.
@CodeAbyss
Copy link
Contributor Author

@Akasurde It seems the tests have again generated an "AttributeError" error. Can you kindly review the code? I am not able to find the bug. I have alreday put a check 'bool(name)' in place to avoid the call to "NoneType".lower().

@Akasurde
Copy link
Member

@CodeAbyss Want to try this -

    return [p.info['pid'] for p in psutil.process_iter(attrs=['pid', 'name']) if p.info and p.info.get('name', None) and p.info['name'].lower() == name.lower()]

Fixed "AttributeError" for object 'NoneType' while calling lower() on the returned process info from psutil's process_iter. 
Thanks to @Akasurde for suggestion :)
@ansibot
Copy link
Contributor

ansibot commented Feb 19, 2019

@CodeAbyss This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

Added check on p.info and p.info['name'] to check if they are Empty or None, and then calling lower() on them.

The older commit message contained the mentions, so commiting new with changed commit message.
@ansibot

This comment has been minimized.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Feb 20, 2019
Removed whitespaces. Added them earlier to amend the previous commit message.
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 20, 2019
@gundalow gundalow changed the title case insensitive string comparison for process names pids: case insensitive string comparison for process names Feb 21, 2019
Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

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

LGTM

@gundalow
Copy link
Contributor

@CodeAbyss Can you please squash your commits to get rid of the Thanks to @Akasurde for suggestion :) change to Thanks to Akasurde for suggestion :) (without @)

The reason for this is GitHub will send him an email every time someone merges this

@gundalow gundalow self-assigned this Feb 22, 2019
@CodeAbyss
Copy link
Contributor Author

@gundalow I might sound naive, but I don't know how to squash the commits. I searched, but all of them are telling cmd line params for git. Can you guide me on how to do this on GitHub?

@Akasurde
Copy link
Member

@CodeAbyss You need to following

  1. You have 5 commits
* 755e839e4d - (HEAD -> patch-2, CodeAbyss/patch-2) Removed Whitespaces (5 days ago) <abyss> GitHub
* 9290e7b151 - Fixed "AttributeError" for 'NoneType'.lower() (5 days ago) <abyss> GitHub
* 5966f77172 - Fixed "AttributeError" for 'None' process name (6 days ago) <abyss> GitHub
* 5818be60cf - Fixed 'AttributeError' for lower() (6 days ago) <abyss> GitHub
* 549ff8bfb4 - case insensitive string comparison for process names (6 days ago) <abyss> GitHub
  1. git rebase -i HEAD~5 - This will rebase your branch (squashing)
pick 549ff8bfb4 case insensitive string comparison for process names
squash 5818be60cf Fixed 'AttributeError' for lower()
squash 5966f77172 Fixed "AttributeError" for 'None' process name
squash 9290e7b151 Fixed "AttributeError" for 'NoneType'.lower()
squash 755e839e4d Removed Whitespaces
  1. Save your changes and amend commit message where it says name in commit message.

  2. git push origin patch-2 --force

You can read about interactive rebase here - https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history and https://git-scm.com/book/en/v2/Git-Branching-Rebasing

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 5, 2019
@Akasurde
Copy link
Member

Akasurde commented Mar 8, 2019

@CodeAbyss Any news ? Let me know if you want me to take over this PR and get it merged. Thanks.

@CodeAbyss
Copy link
Contributor Author

@Akasurde I have followed the steps as per your instructions & below are the details. Kindly have a look::

git log
commit ef523e10a86281211b602a896d58c4a4b3a37d53 (HEAD -> devel)
Author: abyss
Date:   Tue Feb 19 16:40:27 2019 +0530
    case insensitive string comparison for process names
    Fixed 'AttributeError' for lower()
    Fixed "AttributeError: 'NoneType' object has no attribute 'lower'" error for "Empty" or None strings passed as process name.
    Fixed "AttributeError" for 'None' process name
    Fixed "AttributeError" for object 'NoneType' while calling lower() on the returned process info from psutil's process_iter.
    Thanks to Akasurde for suggestion :)
    Fixed "AttributeError" for 'NoneType'.lower()
    Added check on p.info and p.info['name'] to check if they are Empty or None, and then calling lower() on them.
    The older commit message contained the mentions, so commiting new with changed commit message.
    Removed Whitespaces
    Removed whitespaces. Added them earlier to amend the previous commit message.

git push origin patch-2 --force
Everything up-to-date

git push
Enumerating objects: 13, done.
Counting objects: 100% (13/13), done.
Delta compression using up to 2 threads
Compressing objects: 100% (6/6), done.
Writing objects: 100% (7/7), 953 bytes | 476.00 KiB/s, done.
Total 7 (delta 5), reused 1 (delta 0)
remote: Resolving deltas: 100% (5/5), completed with 5 local objects.
To https://github.com/CodeAbyss/ansible.git
   57d85031d7..ef523e10a8  devel -> devel

Is that it or do I have to create a new PR?

@Akasurde Akasurde merged commit e428441 into ansible:devel Mar 14, 2019
@CodeAbyss CodeAbyss deleted the patch-2 branch March 15, 2019 07:15
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. system System category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pids is returning empty list for existing running process
5 participants