Mail Job Notification Plugin#5943
Conversation
richtja
left a comment
There was a problem hiding this comment.
Hi @harvey0100, thank you for your contribution. It is very welcome. I haven't looked at the code yet. But I can give you some comments which should help you to solve the CI failures:
- In your git config your email is harveylynden@gmail.com and not hlynden@redhat.com therefore the signedoff-check is failing. You need to change your git config and create a new commit with the proper sign.
- Some static checks are failing, please fix them base on the checks output. You can run those tests locally by setup.py test --select=static-checks, and it is recommended to run the tests before you push your changes.
- The Readme file has some syntax errors. Therefore, you have the Build Package failures.
- The Linux with Python 3.12.0 is not related to your PR and I have already created a fix for that in #5942
I hope that it will help you. If you have any questions, don't hesitate to ask.
0c0e5cc to
ff8c241
Compare
clebergnu
left a comment
There was a problem hiding this comment.
Hi @harvey0100,
Thanks for this! Please find my initial comments inline in the changes.
a4624f6 to
eb973ff
Compare
39830c7 to
1298f15
Compare
richtja
left a comment
There was a problem hiding this comment.
Hi @harvey0100, thank you very much for the CI fixes. I have more deeply looked at the code and I have a few proposals which IMO increase readability and maintainability of the code. Please let me know what do you think about it.
Also, can you please write a more detailed commit message. For example, you can use your PR description. Thanks.
| def _generate_test_summary(data, detail_level): | ||
| test_summary = [] | ||
|
|
||
| def format_test_details(test): |
There was a problem hiding this comment.
Is there any reason for this inner method, instead of calling Mail._format_test_details(test, advanced=detail_level) directly?
2ea1b49 to
7fcc607
Compare
3d073a7 to
4a7f518
Compare
d37f13f to
37aaeb2
Compare
131b737 to
1dfb081
Compare
clebergnu
left a comment
There was a problem hiding this comment.
Hi @harvey0100 ,
There are a few bits missing to have this properly packaged as an RPM:
diff --git a/python-avocado.spec b/python-avocado.spec
index 4eae27dba..5f71e4f20 100644
--- a/python-avocado.spec
+++ b/python-avocado.spec
@@ -135,6 +135,9 @@ popd
pushd optional_plugins/result_upload
%py3_build
popd
+pushd optional_plugins/mail
+%py3_build
+popd
%if ! 0%{?rhel}
pushd optional_plugins/spawner_remote
%py3_build
@@ -175,6 +178,9 @@ popd
pushd optional_plugins/result_upload
%py3_install
popd
+pushd optional_plugins/mail
+%py3_install
+popd
%if ! 0%{?rhel}
pushd optional_plugins/spawner_remote
%py3_install
@@ -239,6 +245,7 @@ PATH=%{buildroot}%{_bindir}:%{buildroot}%{_libexecdir}/avocado:$PATH \
%exclude %{python3_sitelib}/avocado_varianter_pict*
%exclude %{python3_sitelib}/avocado_varianter_cit*
%exclude %{python3_sitelib}/avocado_result_upload*
+%exclude %{python3_sitelib}/avocado_result_mail*
%exclude %{python3_sitelib}/avocado_framework_plugin_result_html*
%exclude %{python3_sitelib}/avocado_framework_plugin_resultsdb*
%exclude %{python3_sitelib}/avocado_framework_plugin_varianter_yaml_to_mux*
@@ -247,6 +254,7 @@ PATH=%{buildroot}%{_bindir}:%{buildroot}%{_libexecdir}/avocado:$PATH \
%exclude %{python3_sitelib}/avocado_framework_plugin_golang*
%exclude %{python3_sitelib}/avocado_framework_plugin_ansible*
%exclude %{python3_sitelib}/avocado_framework_plugin_result_upload*
+%exclude %{python3_sitelib}/avocado_framework_plugin_result_mail*
%exclude %{python3_sitelib}/avocado_framework_plugin_spawner_remote*
%exclude %{python3_sitelib}/tests*Please include these changes and let's see what packit gives us.
77d4734 to
8d79ddb
Compare
Hi Cleber, Thanks for the changes, I've implemented them and looks like the build is succeding! :) |
clebergnu
left a comment
There was a problem hiding this comment.
LGTM, thanks!
(waiving the codeclimate "failure")
richtja
left a comment
There was a problem hiding this comment.
Hi @harvey0100, thank you for your updates. IMO it is almost finished, I have just few style comments.
54b7d52 to
adb7684
Compare
|
All changes have been applied that have been requested, request should be ready for merging. |
optional_plugins/mail/MANIFEST.in
Outdated
| @@ -0,0 +1 @@ | |||
| include VERSION README.rst No newline at end of file | |||
There was a problem hiding this comment.
I'll get there one day haha, IDE is now set to save all files with a new line :D - sorted now
Added sphinx target inside configuring.rst Created symbolic link inside of documentation Plugin name finalised Altered towards comments Signed-off-by: Harvey Lynden <hlynden@redhat.com>
adb7684 to
350305a
Compare
richtja
left a comment
There was a problem hiding this comment.
After the updates it LGTM, thank you.
Gmail Plugin for Avocado allows results to be sent to any email. The Origin email though at this time has to come from a google account due to using App Passwords.
PIP package has also been created
Signed off by: Harvey Lynden hlynden@redhat.com