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

Fix Logging in ManageOperators Class #10

Merged
merged 4 commits into from
Apr 7, 2021

Conversation

mcdonnnj
Copy link
Member

@mcdonnnj mcdonnnj commented Apr 6, 2021

🗣 Description

This PR fixes an issue with a logging message and makes some small edits to improve readability and consistency.

💭 Motivation and context

I was testing a bash script that uses this today and when using the add command twice ran into an error with logging output:

--- Logging error ---
Traceback (most recent call last):
  File "/Users/mcdonnnj/.pyenv/versions/3.9.1/lib/python3.9/logging/__init__.py", line 1079, in emit
    msg = self.format(record)
  File "/Users/mcdonnnj/.pyenv/versions/3.9.1/lib/python3.9/logging/__init__.py", line 923, in format
    return fmt.format(record)
  File "/Users/mcdonnnj/.pyenv/versions/3.9.1/lib/python3.9/logging/__init__.py", line 659, in format
    record.message = record.getMessage()
  File "/Users/mcdonnnj/.pyenv/versions/3.9.1/lib/python3.9/logging/__init__.py", line 363, in getMessage
    msg = msg % self.args
TypeError: not all arguments converted during string formatting
Call stack:
  File "/Users/mcdonnnj/.pyenv/versions/manage-cyhy-ops/bin/manage-cyhy-ops", line 33, in <module>
    sys.exit(load_entry_point('manage-cyhy-ops', 'console_scripts', 'manage-cyhy-ops')())
  File "/Volumes/workspace/my-repos/manage-cyhy-ops/src/manage_cyhy_ops/cli.py", line 151, in main
    manager.add_user(username, ssh_key, overwrite=overwrite_ssh_key)
  File "/Volumes/workspace/my-repos/manage-cyhy-ops/src/manage_cyhy_ops/manageoperators.py", line 140, in add_user
    return self._update_cyhy_ops_users(username)
  File "/Volumes/workspace/my-repos/manage-cyhy-ops/src/manage_cyhy_ops/manageoperators.py", line 91, in _update_cyhy_ops_users
    logging.info(log_msg, username, self.region)
Message: 'Successfully performed no operations on CyHy Operators in region "%s"'
Arguments: ('first.last', 'us-east-1')

While fixing that issue I improved the output phrasing and made changes to keep all the logging consistent as a result.

🧪 Testing

Automated testing passes. I verified that the modified version does not exhibit the error mentioned above.

WARNING SSH key for "first.last" already exists in the Parameter Store for region "us-east-1".
WARNING If you need to overwrite this value, please use the "--overwrite" switch.
WARNING User "first.last" is already in the list of active CyHy Operators in region "us-east-1".
INFO Performed no operations for "first.last" in region "us-east-1"

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

The default value does not include an operator for the user value we pass in
the logging output call. This results in a TypeError exception because there
are more arguments than the format string has operators.
Since "Successfully" was removed to improve readability for the output in
ManageOperators._update_cyhy_ops_users(), I have mirrored that change in the
rest of the class.
@mcdonnnj mcdonnnj added the bug This issue or pull request addresses broken functionality label Apr 6, 2021
@mcdonnnj mcdonnnj self-assigned this Apr 6, 2021
@mcdonnnj mcdonnnj marked this pull request as ready for review April 6, 2021 22:20
@mcdonnnj mcdonnnj requested review from felddy and jsf9k April 6, 2021 22:20
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

LGTM! ⛔ 🐞
Please note my single-character suggestion.

src/manage_cyhy_ops/manageoperators.py Outdated Show resolved Hide resolved
mcdonnnj and others added 2 commits April 7, 2021 12:57
Consistency is key.

Co-authored-by: dav3r <david.redmin@trio.dhs.gov>
Further tweak the logging messages to ensure they are consistent in structure
and language.
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcdonnnj mcdonnnj requested a review from dav3r April 7, 2021 19:12
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Solid! 🎸

@mcdonnnj mcdonnnj merged commit efe91cb into develop Apr 7, 2021
@mcdonnnj mcdonnnj deleted the bug/fix_bad_logging_statement branch April 7, 2021 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants