<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Creating PR directly is IMHO fine, you can get better feedback there pinpointing to particular lines etc. - a code review is better to be done in github PR than via mail.<br><br></div><div class="gmail_default" style="font-family:monospace,monospace">Basically plugin seems ok, few points:<br><br></div><div class="gmail_default" style="font-family:monospace,monospace">- pep8 formatting :)<br></div><div class="gmail_default" style="font-family:monospace,monospace">- instead of pure URL with mssql.conf format, it is (also) worth explaining what the further code performs<br></div><div class="gmail_default" style="font-family:monospace,monospace">- catch exception when the config file is not present or readable<br></div><div class="gmail_default" style="font-family:monospace,monospace">- aren't you interested in collecting the mssql_conf file itself (but if so, can't it contain some credentials we should obfuscate? if so, see e.g. <a href="https://github.com/sosreport/sos/blob/master/sos/plugins/openstack_ansible.py#L30">https://github.com/sosreport/sos/blob/master/sos/plugins/openstack_ansible.py#L30</a> how to do that)<br><br><br></div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><font face="monospace,monospace">Kind regards,<br></font></div><font face="monospace,monospace">Pavel<br></font></div></div></div>
<br><div class="gmail_quote">On Thu, May 24, 2018 at 9:45 AM, Takayoshi Tanaka <span dir="ltr"><<a href="mailto:tatanaka@redhat.com" target="_blank">tatanaka@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hello,<br clear="all"><div><br></div><div>I'm working on Azure support team and now writing a plugin for $subject.</div><div>Since I'm a beginner of Python, I would appreciate your comments before I submit a PR.</div><div>Here is a source code.</div><div><a href="https://github.com/tanaka-takayoshi/sos/blob/mssql/sos/plugins/mssql.py" target="_blank">https://github.com/tanaka-<wbr>takayoshi/sos/blob/mssql/sos/<wbr>plugins/mssql.py</a><br></div><div><br></div><div>I'm concerned with a way to parse configuration file to specify the log file location.</div><div>The location of log files is specified in mssql.conf, which is located at /var/opt/mssql/mssql.conf by default.</div><div>This file looks like as follows.</div><div><br></div><div>```</div><div><div>[filelocation]<br></div><div>errorlogfile = /var/opt/mssql/log<br></div><div><br></div><div>[sqlagent]</div><div>databasemailprofile = default</div><div>errorlogfile = /var/opt/mssql/log/<wbr>sqlagentlog.log</div></div><div>```</div><div><br></div><div>I'm parsing this file in this location.</div><div><a href="https://github.com/tanaka-takayoshi/sos/blob/mssql/sos/plugins/mssql.py#L35-L48" target="_blank">https://github.com/tanaka-<wbr>takayoshi/sos/blob/mssql/sos/<wbr>plugins/mssql.py#L35-L48</a><br></div><div><br></div><div>Is this a good way to parse this conf file in Python/sos plugin?</div><div>Also, I would appreciate any comments on my code. </div><div><br></div><div><br></div><div>Thanks,</div>-- <br><div class="m_-7748419568644509153gmail_signature"><div dir="ltr"><p style="color:rgb(0,0,0);font-family:overpass,sans-serif;font-weight:bold;margin:0px;padding:0px;font-size:14px;text-transform:uppercase"><span>TAKAYOSHI</span> <span>TANAKA</span></p><p style="color:rgb(0,0,0);font-family:overpass,sans-serif;font-size:10px;margin:0px 0px 4px;text-transform:uppercase"><span>SOFTWARE MAINTENANCE ENGINEER</span><span style="color:rgb(204,204,204)">, <span style="color:rgb(170,170,170);margin:0px">RHCA, MICROSOFT MVP</span></span></p><p style="font-family:overpass,sans-serif;margin:0px;font-size:10px;color:rgb(153,153,153)"><a href="https://www.redhat.com/" style="color:rgb(0,136,206);margin:0px" target="_blank">Red Hat <span>K.K.</span></a></p><span style="font-family:overpass,sans-serif;font-size:10px;margin:0px;color:rgb(153,153,153)"><p style="margin:0px">8F, Ebisu Neonato, 4-1-18 Ebisu</p></span><span style="color:rgb(0,0,0);font-family:overpass,sans-serif;font-size:medium"></span><span style="color:rgb(0,0,0);font-family:overpass,sans-serif;font-size:medium"><p style="font-size:10px;margin:0px;color:rgb(153,153,153)">Shibuya-ku Tokyo, 150-0013, Japan</p></span><span style="color:rgb(0,0,0);font-family:overpass,sans-serif;font-size:medium"></span><p style="font-family:overpass,sans-serif;margin:0px 0px 6px;font-size:10px;color:rgb(153,153,153)"><span style="margin:0px;padding:0px"><a href="mailto:tatanaka@redhat.com" style="color:rgb(0,136,206);margin:0px" target="_blank">tatanaka@redhat.com</a>   </span> <span>M: <a href="tel:+81-80-4193-5143" style="color:rgb(0,136,206);font-size:11px;margin:0px" target="_blank">+81-80-4193-5143</a>    </span> <span>IM: <span>tatanaka</span></span></p><table style="color:rgb(0,0,0);font-family:overpass,sans-serif;font-size:medium" border="0"><tbody><tr><td width="100px"><a href="https://red.ht/sig" target="_blank"><img src="https://www.redhat.com/files/brand/email/sig-redhat.png" height="auto" width="90"></a></td></tr></tbody></table></div></div>
</div>
<br>______________________________<wbr>_________________<br>
sos-devel mailing list<br>
<a href="mailto:sos-devel@redhat.com">sos-devel@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/sos-devel" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/sos-devel</a><br></blockquote></div><br></div>