Fixes #39 - raise warning on unread mails #56

kgaikwad · 2017-04-24T10:03:57Z

No description provided.

gnurag · 2017-04-25T13:39:25Z

definitions/checks/unread_mail.rb

+  attr_reader :file_path
+
+  def initialize
+    @file_path = '/var/spool/mail/root'


Would it be feasible to retrieve mail spool path from site-specific configuration file /etc/login.defs? This file defines the default path for MAIL_DIR, and its value differs based on distro.

On Fedora*

[anurag@coruscant ~]$ cat /etc/redhat-release Fedora release 25 (Twenty Five) [anurag@coruscant ~]$ grep ^MAIL_DIR /etc/login.defs MAIL_DIR /var/spool/mail [anurag@coruscant ~]$

On Debian*

anurag@ellen:~$ cat /etc/debian_version wheezy/sid anurag@ellen:~$ grep ^MAIL_DIR /etc/login.defs MAIL_DIR /var/mail anurag@ellen:~$

You may want to retain the default value of /var/spool/mail/root and fallback to it if the above queries fail.

@gnurag,
Thank you for providing information.
I will go through it and fix the code as per your suggestions.

@gnurag ,
Done with the suggested changes.
Instead of hard-coded path, value of MAIL_DIR is set as a mailbox path.
This value of MAIL_DIR is retrieved from config file /etc/login.defs .

gnurag · 2017-04-25T13:39:41Z

definitions/checks/unread_mail.rb

+  end
+
+  def number_of_mails_present
+    cmd = "egrep '^Message-Id' #{file_path} | wc -l"


egrep --count can provide a count of matching lines, without passing the output to wc for counting.

iNecas · 2017-06-07T20:18:28Z

definitions/checks/unread_mail.rb

+  def guidline_steps
+    <<-EOF
+    Please follow steps manually to clear root mailbox i.e #{file_path}:
+    1) To read mails use vim or cat for mailbox file


What about defining this steps into procedures/system_mailbox and linking to them from the check
in assertion: this would be really nice user experience, if we listed the file for them, as well as allowed them
to delete that file.

kgaikwad · 2017-06-29T07:19:55Z

@iNecas, @gnurag, @swapab,
Done with the suggested changes. Ready for review.

Converted manual text steps to view & delete unread mail list to procedures
Added error_type key in options for method assert. So that instead of fail, we can pass and print other error types too.

swapab · 2017-06-29T10:20:35Z

I think, there should be a mail feature. And check should be for any new mail?.
It would be a structural change with all the check logic moving to mail feature and call feature(:mail).new_mail? from the check.

kgaikwad · 2017-06-29T14:48:15Z

@swapab,
I don't think to create mail as feature would be needed here.
There will be very less chance where future tasks will be related to system mails.
Due to this reason only I have not created mail as feature separately.

If you still think, this change should be needed, please let me know.
I will do modifications and update PR.

swapab · 2017-07-04T12:58:06Z

definitions/checks/unread_mail.rb

+
+  def run
+    if file_exists?(file_path)
+      mails_count = find_mails_count.to_i


can move to_i to the method itself.
Method name can be mails_count.

swapab · 2017-07-04T12:59:27Z

can keep the check as is. we'll introduce mail feature when firm use-case arrives.

kgaikwad · 2017-07-05T06:37:37Z

Ready for review.
As I am not using any mail client package and there is no any info about any mail which says it is unread,
I am showing all mails count from file /var/spool/mail/root .

kgaikwad · 2017-08-22T06:29:52Z

lib/foreman_maintain/check.rb

@@ -15,18 +15,14 @@ class Check < Executable
    #                   the failure, will be offered to the user when running
    #                   in interactive mode
    #
-    # * +:warn* - issue warning instead of failure: this is less strict check,
-    #      that could be considered as non-critical for continuing with the scenario
+    # * +:error_type* - error type if wants to override failure


@iNecas , @swapab
Here, I have replaced :warn option to :error_type.
Using this option, we can raise warning or any other error_type of exceptions.

kgaikwad · 2017-08-22T06:30:42Z

@iNecas ,
Resolved conflicts.

swapab · 2017-09-08T07:39:34Z

@kgaikwad I tried this on my local setup.

# ./bin/foreman-maintain health check
Running ForemanMaintain::Scenario::FilteredScenario
================================================================================
Check for verifying syntax for ISP DHCP configurations:         
Not Found 404. Response: ""
              [FAIL]
Please check and verify DHCP configurations.
--------------------------------------------------------------------------------
check for paused tasks:                                               [OK]
--------------------------------------------------------------------------------
Check for mails in root's mailbox.:                                   [WARNING]
WARNING: Found 20 mail(s) in root's mailbox.
--------------------------------------------------------------------------------
There are multiple steps to proceed:
1) list mails from root's mailbox ({:file_path=>#<ForemanMaintain::Param:0x0000000194d8c8 @name=:file_path, @description="", @options={}, @required=false, @flag=false, @block=nil, @array=false>})
2) clear root's mailbox ({:file_path=>#<ForemanMaintain::Param:0x0000000194fbc8 @name=:file_path, @description="", @options={}, @required=false, @flag=false, @block=nil, @array=false>})
Select step to continue, [n(next), q(quit)] q
Scenario [ForemanMaintain::Scenario::FilteredScenario] failed.

I think the key :file_path should have a String as value instead of ForemanMaintain::Param object.

kgaikwad · 2017-09-08T12:43:15Z

@swapab ,
I guess something is broken in my code due to new changes, I will check on this.

kgaikwad · 2017-09-12T12:17:57Z

@swapab ,

Sorry for confusion nothing is broken so far :-)
This param list shown along with procedure's description due to method runtime_message

I have overridden this method in both procedures. Now, it will show only procedure's description for this check.

kgaikwad · 2017-09-12T15:59:35Z

@swapab ,

Reverted the previous changes as fixed in PR-97.

kgaikwad · 2017-09-13T10:46:27Z

Pulled new changes from master and updated the PR.

ntkathole · 2018-02-28T08:25:03Z

@kgaikwad Testing against satellite 6.3 on RHEL 7 gives some error on my setup

./bin/foreman-maintain advanced procedure run system-mailbox-clear-mailbox
Running ForemanMaintain::Scenario
================================================================================
clear root's mailbox:
/ clearing all existing mailssh: -c: line 0: syntax error near unexpected token `2'
sh: -c: line 0: `cat /dev/null >  2>&1'
                                         [OK]
--------------------------------------------------------------------------------

and also running system-mailbox-list-mailbox stucked (never ends)

./bin/foreman-maintain advanced procedure run system-mailbox-list-mailbox
Running ForemanMaintain::Scenario
================================================================================
list mails from root's mailbox:

theforeman-bot · 2018-03-02T19:21:53Z

@kgaikwad, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #39: Node Authentication is the correct one
move ticket #39 from Foreman to the Foreman Maintain project
or file a new ticket in the Foreman Maintain project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

sean797 · 2018-03-16T18:46:15Z

Whats the motivation behind this? Can we at least add it to the commit message please.

kgaikwad · 2018-03-16T19:15:42Z

@sean797, updated the commit message.

@ntkathole , added advanced_run metadata to above procedures and updated the pull-request.

ntkathole · 2018-03-19T06:56:25Z

definitions/checks/unread_mail.rb

+  metadata do
+    label :unread_mail
+    description "Check for mails in root's mailbox."
+    tags :unread_mail


@kgaikwad We are tagging this check with unread_mail...Currently this check won't run before upgrade unless user run it explicitly ( foreman-maintain health check --label unread-mail ). So I think to this need to add in scenario definitions https://github.com/theforeman/foreman_maintain/blob/master/definitions/scenarios/upgrade_to_satellite_6_3.rb#L23

add_step(Checks:::UnreadMail.new)

@ntkathole ,
It's not intended to be a pre/post upgrade checks. That's why tagged as unread_mail so that any user can run it explicitly whenever required.

mbacovsky · 2018-04-11T08:47:20Z

definitions/checks/unread_mail.rb

+  end
+
+  private
+


@kgaikwad, would it make sense to make file_path and mailbox_dir lazy so that they are evaluated during the run instead of initialization? I see two advantages there:

it takes time during scenario execution when there is already something printed out and scenario can start quicker

if (hypothetically) if file_path is subject of change in some of the previous steps we won't notice

Yes, make sense! And advantages mentioned are legit.
I will do modifications as suggested.

We have many crons, they end up in root mail folder (or file technically). With this commit it will check for unread mails from root's mailbox. If unread mails, provide steps to list & clear mailbox.

kgaikwad · 2018-04-20T11:07:33Z

@mbacovsky, @ntkathole,
Updated the pull-request as per suggested changes.

ntkathole · 2018-04-20T13:04:03Z

Looks good!

iNecas

Approved: @mbacovsky feel free to merge unless any more comments.

sean797 · 2018-06-12T14:17:47Z

Whats the motivation behind this? I did ask already but it seems the commit message was updated telling me what this commit does (which is useful 👍 ) but the motivation behind why we are doing it would be nice.

Right now, I'm thinking why the hell would we want to warn users about unread mails in root's mailbox!!?

from man cron "When executing commands, any output is mailed to the owner" this means foreman-maintain is going to warn users that they have cronjobs ran that produced output, I'm wondering why that is useful?

kgaikwad · 2018-06-12T17:53:03Z

@sean797,
Initially, this check was suggested by @lzap so he can provide better insights.

lzap · 2018-06-13T13:07:25Z

I think I was brainstorming but the original ticket is no longer accessible probably because folks disabled issues in this repo - don't know the details. I guess it won't hurt to warn user, but the amount of code written to do one-line check is just worrisome :-) If anyone told me in advance we are gonna deliver 100 lines of code for this, I'd say: please no, just close the idea.

upadhyeammit · 2020-09-21T12:22:40Z

As per the recent comment of @lzap and @sean797 I am closing this pr.

gnurag reviewed Apr 25, 2017

View reviewed changes

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch 2 times, most recently from a77cbac to 30e445b Compare April 26, 2017 10:59

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch 2 times, most recently from f5cc6bd to 970843c Compare May 4, 2017 08:27

iNecas reviewed Jun 7, 2017

View reviewed changes

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch 2 times, most recently from 2d8143e to b22f01f Compare June 29, 2017 07:17

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from b22f01f to cab5e4c Compare June 29, 2017 12:17

swapab reviewed Jul 4, 2017

View reviewed changes

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from cab5e4c to aa80b53 Compare July 5, 2017 05:02

swapab added Needs re-review Needs testing labels Aug 10, 2017

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from aa80b53 to 0c6ad0c Compare August 22, 2017 06:22

kgaikwad commented Aug 22, 2017

View reviewed changes

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from 0c6ad0c to 9b26a52 Compare September 8, 2017 12:37

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from 9b26a52 to ba5a302 Compare September 12, 2017 12:12

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from ba5a302 to ad0adb8 Compare September 12, 2017 15:58

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from ad0adb8 to fee376e Compare September 13, 2017 10:44

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from fee376e to 76aea88 Compare March 2, 2018 19:21

theforeman-bot added the Waiting on contributor label Mar 2, 2018

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 2, 2018

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from 76aea88 to 10a4185 Compare March 2, 2018 19:29

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch 2 times, most recently from 0679740 to 9b8e044 Compare March 16, 2018 19:12

ntkathole reviewed Mar 19, 2018

View reviewed changes

mbacovsky reviewed Apr 11, 2018

View reviewed changes

Fixes #20148 - raise warning on unread mails

7030fcf

We have many crons, they end up in root mail folder (or file technically). With this commit it will check for unread mails from root's mailbox. If unread mails, provide steps to list & clear mailbox.

kgaikwad force-pushed the 39_check_if_root_having_unread_mails branch from 9b8e044 to 7030fcf Compare April 20, 2018 11:04

iNecas approved these changes Apr 23, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 23, 2018

upadhyeammit closed this Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #39 - raise warning on unread mails #56

Fixes #39 - raise warning on unread mails #56

kgaikwad commented Apr 24, 2017

gnurag Apr 25, 2017

kgaikwad Apr 26, 2017

kgaikwad Apr 26, 2017

gnurag Apr 28, 2017

gnurag Apr 25, 2017

iNecas Jun 7, 2017

kgaikwad commented Jun 29, 2017

swapab commented Jun 29, 2017

kgaikwad commented Jun 29, 2017

swapab Jul 4, 2017

swapab commented Jul 4, 2017 •

edited

kgaikwad commented Jul 5, 2017

kgaikwad Aug 22, 2017

kgaikwad commented Aug 22, 2017

swapab commented Sep 8, 2017

kgaikwad commented Sep 8, 2017

kgaikwad commented Sep 12, 2017

kgaikwad commented Sep 12, 2017

kgaikwad commented Sep 13, 2017

ntkathole commented Feb 28, 2018

theforeman-bot commented Mar 2, 2018

sean797 commented Mar 16, 2018

kgaikwad commented Mar 16, 2018

ntkathole Mar 19, 2018

kgaikwad Apr 10, 2018

mbacovsky Apr 11, 2018

kgaikwad Apr 11, 2018

kgaikwad commented Apr 20, 2018

ntkathole commented Apr 20, 2018

iNecas left a comment

sean797 commented Jun 12, 2018

kgaikwad commented Jun 12, 2018

lzap commented Jun 13, 2018

upadhyeammit commented Sep 21, 2020

Fixes #39 - raise warning on unread mails #56

Fixes #39 - raise warning on unread mails #56

Conversation

kgaikwad commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Jun 29, 2017

swapab commented Jun 29, 2017

kgaikwad commented Jun 29, 2017

Choose a reason for hiding this comment

swapab commented Jul 4, 2017 • edited

kgaikwad commented Jul 5, 2017

Choose a reason for hiding this comment

kgaikwad commented Aug 22, 2017

swapab commented Sep 8, 2017

kgaikwad commented Sep 8, 2017

kgaikwad commented Sep 12, 2017

kgaikwad commented Sep 12, 2017

kgaikwad commented Sep 13, 2017

ntkathole commented Feb 28, 2018

theforeman-bot commented Mar 2, 2018

sean797 commented Mar 16, 2018

kgaikwad commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Apr 20, 2018

ntkathole commented Apr 20, 2018

iNecas left a comment

Choose a reason for hiding this comment

sean797 commented Jun 12, 2018

kgaikwad commented Jun 12, 2018

lzap commented Jun 13, 2018

upadhyeammit commented Sep 21, 2020

swapab commented Jul 4, 2017 •

edited