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.

Sign up for GitHub

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

Jump to bottom

Add exit_code to the messages doc and some logging improvements #314

Merged
merged 17 commits into from
Oct 17, 2022
Merged

Add exit_code to the messages doc and some logging improvements #314

merged 17 commits into from
Oct 17, 2022

Conversation

PhillypHenning
Copy link
Contributor

@PhillypHenning PhillypHenning commented Oct 7, 2022 •
edited by arm4b
Loading

Description

Adds file checking and some overall logging improvements.

Fixes #310
Closes #311

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update

How Has This Been Tested?

Logs

#~#~#~#~#~#~#~#~#~#~#~#~#~#~#~#~#~#~#~# 
                                
2022-10-07 16:19:44,488 root         INFO loading config file: [/tmp/tmp0a_vtg_z/AwsAnsible/ansible/bitops.config.yaml]
2022-10-07 16:19:44,488 root         DEBUG loading ENV_FILE   : [/tmp/tmp0a_vtg_z/AwsAnsible/ansible/ENV_FILE]
2022-10-07 16:19:44,489 root         ERROR 
        Required file was not found. To fix this please add the following file: [/opt/bitops/scripts/plugins/ansible/plugin.config.yaml]

2022-10-07 16:48:55,276 root         ERROR 
        Required file was not found. To fix this please add the following file: [/tmp/tmph3t8r6ji/AwsAnsible/ansible/bitops.config.yaml]

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

arm4b
arm4b requested changes Oct 10, 2022
View reviewed changes
Copy link
Member

@arm4b arm4b left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR!

I left some notes to address.
Comments with the FYI note are not required to be fixed, but are nice to haves if you'd like to make those changes so I won't need to fix them in #298.

Could you also provide a bit more specific PR title? After all, the PR titles will be used for auto-generating the Release Notes for the upcoming v2.1.0. Sounds like we're mixing both enhancement + fix in this PR.

scripts/plugins/deploy_plugins.py Outdated Show resolved Hide resolved
scripts/plugins/documentation.json Outdated Show resolved Hide resolved
scripts/plugins/doc.py Outdated Show resolved Hide resolved
scripts/plugins/documentation.json Outdated Show resolved Hide resolved
scripts/plugins/install_plugins.py
@@ -126,13 +126,10 @@ def Install_Plugins():
)

except FileNotFoundError as e:
logger.warning(
"No plugin file was found at path: [{}]".format(
Copy link
Member

Choose a reason for hiding this comment

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

In this case, No plugin was found at path: was more specific and helpful, compared to the more universal Required file was not found. To fix this please add the following file:.

If the plugin configuration is missing, it is worth warning that explicitly and providing recommendations what is the plugin configuration file and ideally its format.

Copy link
Member

Choose a reason for hiding this comment

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

@PhillypHenning that's the only missing one.

I think we should try improving the error message clarity by being more specific in this case with the plugin config.

scripts/plugins/utilities.py Outdated Show resolved Hide resolved
@arm4b arm4b added bug 🪲 Something isn't working enhancement ✨ New feature or request labels Oct 10, 2022
@arm4b arm4b added this to the v2.1.0 milestone Oct 10, 2022
arm4b
arm4b reviewed Oct 12, 2022
View reviewed changes
bitops.config.yaml Outdated Show resolved Hide resolved
arm4b
arm4b reviewed Oct 12, 2022
View reviewed changes
scripts/plugins/doc.py Outdated Show resolved Hide resolved
arm4b
arm4b reviewed Oct 13, 2022
View reviewed changes
scripts/plugins/install_plugins.py Outdated
)
)
msg, _ = Get_Doc("missing_optional_file")
logger.warning("{} [{}]".format(msg, plugin_configuration_path))
Copy link
Member

@arm4b arm4b Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I also see we moved the warning message to the documentation.json.

Is the idea to move all the warnings to that file in the future or do we plan to do it only for errors? How about info?
@PhillypHenning what's the convention for this?

arm4b
arm4b approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the changes 👍

@arm4b arm4b changed the title Logging update Add exit_code to the messages doc Oct 16, 2022
@arm4b arm4b changed the title Add exit_code to the messages doc Add exit_code to the messages doc and some logging improvements Oct 17, 2022
@arm4b arm4b merged commit 67a64c5 into main Oct 17, 2022
@arm4b arm4b deleted the logging-update branch October 17, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arm4b arm4b arm4b approved these changes

Assignees
No one assigned
Labels
bug 🪲 Something isn't working enhancement ✨ New feature or request
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.

File check - bitops.config.yaml
2 participants
@PhillypHenning @arm4b