Some corrections #2
Conversation
Signed-off-by: Konstantina Chremmou <[email protected]>
Signed-off-by: Konstantina Chremmou <[email protected]>
Signed-off-by: Konstantina Chremmou <[email protected]>
| @@ -320,7 +320,7 @@ private void Build() | |||
| } | |||
| if (isHost || isPool) | |||
| { | |||
| NRPEEditPage = new NRPEEditPage(isHost); | |||
| NRPEEditPage = new NRPEEditPage(); | |||
There was a problem hiding this comment.
Strictly speaking this is not wrong, but because we have a dedicated method that sets the IXenObject on the panel, it is more tidy if this is examined within that method.
| @@ -75,13 +77,12 @@ public string SubText | |||
| } | |||
| } | |||
|
|
|||
| public Image Image => Images.StaticImages._000_EnablePowerControl_h32bit_16; | |||
| public Image Image => Images.StaticImages._000_Module_h32bit_16; | |||
There was a problem hiding this comment.
The flash icon is not correct in this case. Ideally it would be nice if we had an icon for monitoring resources, but we don't have one at the moment, so I think we can use the module icon which signifies a plugin.
| InitializeComponent(); | ||
| Text = "NRPE"; | ||
| Text = Messages.NRPE; |
There was a problem hiding this comment.
User visible strings should never be hardcoded in the .cs file (even if the text is not translatable in other languages, because in future it may change to something translatable and then we have a localisation bug).
| @@ -192,7 +184,7 @@ public void ShowLocalValidationMessages() | |||
|
|
|||
| public void HideLocalValidationMessages() | |||
| { | |||
| if (InvalidParamToolTip.Tag is Control ctrl && ctrl != null) | |||
| if (InvalidParamToolTip.Tag is Control ctrl) | |||
There was a problem hiding this comment.
The null check at the end is not necessary because the statement is Control ctrl checks implicitly.
| private static readonly Regex REGEX_IPV4 = new Regex("^((25[0-5]|2[0-4]\\d|[01]?\\d\\d?)\\.){3}(25[0-5]|2[0-4]\\d|[01]?\\d\\d?)"); | ||
| private static readonly Regex REGEX_IPV4_CIDR = new Regex("^([0-9]{1,3}\\.){3}[0-9]{1,3}(\\/([0-9]|[1-2][0-9]|3[0-2]))?$"); | ||
| private static readonly Regex REGEX_DOMAIN = new Regex("^(((?!-))(xn--|_)?[a-z0-9-]{0,61}[a-z0-9]{1,1}\\.)*(xn--)?([a-z0-9][a-z0-9\\-]{0,60}|[a-z0-9-]{1,30}\\.[a-z]{2,})$"); | ||
|
|
||
| private readonly bool IsHost = true; | ||
| private bool _isHost; |
There was a problem hiding this comment.
Naming convention: for private instance class fields we use camelcase with undescore. I only changed this instance as an example, but there are several more fields in these files that could benefit from the change.
| @@ -325,24 +322,18 @@ private string AllowHostsWithoutLocalAddress(string allowHosts) | |||
| UpdatedAllowHosts.Substring(0, UpdatedAllowHosts.Length - 1); | |||
| } | |||
|
|
|||
| private void UpdateOtherComponentBasedEnableNRPECheckBox(bool check) | |||
| private void UpdateOtherComponentBasedEnableNRPECheckBox() | |||
There was a problem hiding this comment.
This is an instance method and the parameter passed in it is EnableNRPECheckBox.Checked which can be chekced directly within the method, so there is no need to pass it as a parameter; the code is thus simplified.
| @@ -384,13 +376,20 @@ private void CheckDataGridView_BeginEdit(object sender, DataGridViewCellCancelEv | |||
|
|
|||
| private void CheckDataGridView_EndEdit(object sender, DataGridViewCellEventArgs e) | |||
| { | |||
| DataGridViewCell currentCell = CheckDataGridView.CurrentRow.Cells[e.ColumnIndex]; | |||
| if (!IsHost && currentCell.Value.ToString().Trim().Equals("")) | |||
| DataGridViewCell currentCell = CheckDataGridView.CurrentRow?.Cells[e.ColumnIndex]; | |||
There was a problem hiding this comment.
Null check on the CurrentRow was missing. If it is null, then currentCell will be null, hence we also check below.
|
|
||
| if (checkGroup != null) | ||
| { | ||
| if (currentCell.ColumnIndex == WarningThresholdColumn.Index) |
There was a problem hiding this comment.
When checking the column index it's best to check against WarningThresholdColumn.Index instead of the number, because if we add more columns in future or change the order of columns the code will break.
| <value>Top, Left, Right</value> | ||
| </data> | ||
| <data name="PoolTipsTableLayoutPanel.AutoSize" type="System.Boolean, mscorlib"> | ||
| <data name="EnableNRPECheckBox.AutoSize" type="System.Boolean, mscorlib"> |
There was a problem hiding this comment.
In this file I have made some layout tweaks. The main things to note are:
- I removed the info at the bottom and integrated the information instead in the top label.
- I moved the checkbox outside the groupbox because it is not part of the configuration, but is rather used to switch the feature on/off.
- added hotkey to the controls so one can use the UI only with the keyboard.
- in general the aim is to modify the default values of margins, padding etc. as little as possible.
| @@ -88,7 +88,7 @@ | |||
| <Compile Include="Actions\Host\HostAbstractAction.cs" /> | |||
| <Compile Include="Actions\Host\RebootHostAction.cs" /> | |||
| <Compile Include="Actions\Host\RestartToolstackAction.cs" /> | |||
| <Compile Include="Actions\Host\NRPEUpdateAction.cs" /> | |||
| <Compile Include="Actions\NRPE\NRPEUpdateAction.cs" /> | |||
There was a problem hiding this comment.
I moved these two files together in the same folder. We'll use this folder to also add a new action that will be fetching the configuration as per this comment xenserver#3226 (comment)
| public bool Debug { get => debug; set => debug = value; } | ||
| public bool SslLogging { get => sslLogging; set => sslLogging = value; } | ||
| public Dictionary<string, Check> CheckDict { get => checkDict; } | ||
| public bool EnableNRPE { get; set; } |
There was a problem hiding this comment.
We prefer automatic properties instead of properties with a backing field as they make the code more compact.
|
|
||
| public static readonly string SSL_LOGGING_ENABLE = "0xff"; | ||
| public static readonly string SSL_LOGGING_DISABLE = "0x00"; | ||
| public const string XAPI_NRPE_PLUGIN_NAME = "nrpe"; |
There was a problem hiding this comment.
These can be declared as constants, no need for static declaration.
BengangY
force-pushed
the
private/bengangy/CP-44371
branch
from
96175e1
to
c06defe
Compare
I have made 3 different commits for easier review, feel free to squash them when merging the PR.