Fix SOTA reboot on failure when reboot=no #474
Conversation
|
/review |
Code Review Analysis
Code Review Feedback💡 General suggestions: The PR seems to follow the intended enhancement of handling device reboot configuration. However, it is important to ensure that the new parameter device_reboot is consistently handled across all relevant classes and methods. Additionally, the logic for determining whether to reboot should be encapsulated in a single method to avoid repetition and potential inconsistencies. ✨ Usage tips:
|
| @@ -30,6 +30,9 @@ def reboot(self) -> None: | |||
| logger.debug("") | |||
| self._dispatcher_broker.telemetry("Rebooting ") | |||
| time.sleep(2) | |||
|
|
|||
| def is_reboot(self, device_reboot: str) -> bool: | |||
There was a problem hiding this comment.
Consider moving the is_reboot method to a common utility class or module to avoid duplication across different classes that need to check the reboot condition. [important]
| @@ -41,11 +41,12 @@ class Snapshot(metaclass=ABCMeta): # pragma: no cover | |||
| """ | |||
|
|
|||
| def __init__(self, trtl: Trtl, sota_cmd: str, snap_num: Optional[str], | |||
There was a problem hiding this comment.
Ensure that the device_reboot parameter is documented in the docstring of the Snapshot class constructor to maintain consistency and clarity. [important]
| @@ -125,7 +125,7 @@ def __init__(self, | |||
| # If an exception occurs during string conversion, raise that exception | |||
| raise SotaError('package_list is not a string in manifest') from e | |||
|
|
|||
| self._device_reboot = parsed_manifest['deviceReboot'] | |||
| self._device_reboot = str(parsed_manifest['deviceReboot']) | |||
There was a problem hiding this comment.
Use a more robust method to convert the deviceReboot value to a string that handles potential None or unexpected types gracefully. [important]
| @@ -356,7 +357,7 @@ def execute_from_manifest(self, | |||
| self._update_logger.status = OTA_PENDING | |||
| self._update_logger.error = "" | |||
| self._update_logger.save_log() | |||
| if self.sota_mode == 'download-only' or self._device_reboot in ["No", "N", "n", "no", "NO"]: # pragma: no cover | |||
| if self.sota_mode == 'download-only' or rebooter.is_reboot(self._device_reboot): | |||
There was a problem hiding this comment.
The condition rebooter.is_reboot(self._device_reboot) seems to be inverted. It should not reboot when device_reboot is set to a variant of "no". [important]
| @@ -99,12 +99,14 @@ def create_os_updater(self) -> OsUpdater: | |||
| pass | |||
|
|
|||
| @abstractmethod | |||
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], proceed_without_rollback: bool) -> Snapshot: | |||
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | |||
There was a problem hiding this comment.
Ensure that the device_reboot parameter is passed to all create_snapshotter calls in the os_factory.py file. [important]
| create_snapshotter('update', | ||
| snap_num='1', | ||
| proceed_without_rollback=True, | ||
| ).commit() | ||
| device_reboot='Y').commit() |
There was a problem hiding this comment.
Suggestion: The 'device_reboot' parameter should respect the 'proceed_without_rollback' flag and the 'reboot' setting from the manifest. Hardcoding it to 'Y' may cause unintended reboots when the manifest specifies 'reboot=no'. Consider using a variable to control this behavior based on the manifest settings. [bug]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| create_snapshotter('update', | |
| snap_num='1', | |
| proceed_without_rollback=True, | |
| ).commit() | |
| device_reboot='Y').commit() | |
| create_snapshotter('update', | |
| snap_num='1', | |
| proceed_without_rollback=True, | |
| device_reboot=self._should_reboot()).commit() |
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | ||
| proceed_without_rollback: bool, device_reboot: str) -> Snapshot: | ||
| logger.debug("") | ||
| trtl = Trtl(PseudoShellRunner(), BTRFS) | ||
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, proceed_without_rollback) | ||
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, | ||
| proceed_without_rollback, device_reboot) |
There was a problem hiding this comment.
Suggestion: The 'device_reboot' parameter is a string that likely expects 'yes' or 'no' values. It would be safer to validate this input to ensure it meets expectations and handle any invalid values appropriately. [validation]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | |
| proceed_without_rollback: bool, device_reboot: str) -> Snapshot: | |
| logger.debug("") | |
| trtl = Trtl(PseudoShellRunner(), BTRFS) | |
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, proceed_without_rollback) | |
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, | |
| proceed_without_rollback, device_reboot) | |
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | |
| proceed_without_rollback: bool, device_reboot: str) -> Snapshot: | |
| if device_reboot.lower() not in ['yes', 'no']: | |
| raise ValueError("Invalid value for device_reboot. Expected 'yes' or 'no'.") | |
| logger.debug("Creating snapshotter with device_reboot set to {}".format(device_reboot)) | |
| trtl = Trtl(PseudoShellRunner(), BTRFS) | |
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, | |
| proceed_without_rollback, device_reboot) |
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | ||
| proceed_without_rollback: bool, device_reboot: str) -> Snapshot: | ||
| logger.debug("") | ||
| trtl = Trtl(PseudoShellRunner(), BTRFS) | ||
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, proceed_without_rollback) | ||
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, | ||
| proceed_without_rollback, device_reboot) |
There was a problem hiding this comment.
Suggestion: Consider handling the case where 'device_reboot' is set to 'no' but a reboot is still triggered due to a failure in the update process. This can be done by adding a conditional check before initiating the reboot. [enhancement]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | |
| proceed_without_rollback: bool, device_reboot: str) -> Snapshot: | |
| logger.debug("") | |
| trtl = Trtl(PseudoShellRunner(), BTRFS) | |
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, proceed_without_rollback) | |
| return YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, | |
| proceed_without_rollback, device_reboot) | |
| def create_snapshotter(self, sota_cmd: str, snap_num: Optional[str], | |
| proceed_without_rollback: bool, device_reboot: str) -> Snapshot: | |
| logger.debug("Creating snapshotter with device_reboot set to {}".format(device_reboot)) | |
| trtl = Trtl(PseudoShellRunner(), BTRFS) | |
| snapshotter = YoctoSnapshot(trtl, sota_cmd, self._dispatcher_broker, snap_num, | |
| proceed_without_rollback, device_reboot) | |
| if device_reboot.lower() == 'no': | |
| snapshotter.disable_reboot_on_failure() | |
| return snapshotter |
| logger.debug("") | ||
| self._dispatcher_broker.telemetry("Rebooting ") | ||
| time.sleep(2) |
There was a problem hiding this comment.
Suggestion: The time.sleep(2) call in the reboot method may not be necessary and could be removed unless it serves a specific purpose that is not clear from the context. If it is required for a specific reason, consider adding a comment to explain its purpose. [possible issue]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| logger.debug("") | |
| self._dispatcher_broker.telemetry("Rebooting ") | |
| time.sleep(2) | |
| def reboot(self) -> None: | |
| logger.debug("Rebooting system...") | |
| self._dispatcher_broker.telemetry("Rebooting system...") |
| def is_reboot(self, device_reboot: str) -> bool: | ||
| return False if device_reboot in ["No", "N", "n", "no", "NO"] else True |
There was a problem hiding this comment.
Suggestion: The method is_reboot could be simplified by using the str.lower() method to avoid checking each case variation of "no". This would make the code more maintainable and less error-prone if new variations need to be added in the future. [enhancement]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| def is_reboot(self, device_reboot: str) -> bool: | |
| return False if device_reboot in ["No", "N", "n", "no", "NO"] else True | |
| def is_reboot(self, device_reboot: str) -> bool: | |
| return device_reboot.strip().lower() != "no" |
| @@ -30,6 +30,9 @@ def reboot(self) -> None: | |||
| logger.debug("") | |||
There was a problem hiding this comment.
Suggestion: The debug message logged is empty and should contain meaningful information or be removed if not needed. [enhancement]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| logger.debug("") | |
| def reboot(self) -> None: | |
| logger.debug("Initiating reboot sequence.") |
| dispatcher_state.clear_dispatcher_state() | ||
| if self.snap_num: | ||
| self._rollback_and_delete_snap() | ||
| logger.debug("Rebooting to recover from failed SOTA...") | ||
| rebooter.reboot() | ||
| else: | ||
| time.sleep(time_to_wait_before_reboot) | ||
| logger.debug("Rebooting to recover from failed SOTA...") | ||
| rebooter.reboot() | ||
| time.sleep(time_to_wait_before_reboot) | ||
| if rebooter.is_reboot(self._device_reboot): | ||
| logger.debug("Rebooting to recover from failed SOTA...") | ||
| rebooter.reboot() |
There was a problem hiding this comment.
Suggestion: The recover method in DebianBasedSnapshot class should not attempt to reboot if _device_reboot is set to a value that indicates rebooting is not desired. The current implementation may lead to an unintended reboot. The condition to check rebooter.is_reboot(self._device_reboot) should be moved outside of the else block to ensure it's always evaluated. [bug]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| dispatcher_state.clear_dispatcher_state() | |
| if self.snap_num: | |
| self._rollback_and_delete_snap() | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() | |
| else: | |
| time.sleep(time_to_wait_before_reboot) | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() | |
| time.sleep(time_to_wait_before_reboot) | |
| if rebooter.is_reboot(self._device_reboot): | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() | |
| if self.snap_num: | |
| self._rollback_and_delete_snap() | |
| time.sleep(time_to_wait_before_reboot) | |
| if rebooter.is_reboot(self._device_reboot): | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() |
| self._rollback_and_delete_snap() | ||
| logger.debug("Rebooting to recover from failed SOTA...") | ||
| rebooter.reboot() |
There was a problem hiding this comment.
Suggestion: The recover method in DebianBasedSnapshot class logs the message "Rebooting to recover from failed SOTA..." before actually checking if a reboot is necessary. This message should be logged only when the reboot condition is true to avoid confusion in the logs. [enhancement]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| self._rollback_and_delete_snap() | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() | |
| if self.snap_num: | |
| self._rollback_and_delete_snap() | |
| if rebooter.is_reboot(self._device_reboot): | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() |
| time.sleep(time_to_wait_before_reboot) | ||
| if rebooter.is_reboot(self._device_reboot): | ||
| logger.debug("Rebooting to recover from failed SOTA...") | ||
| rebooter.reboot() |
There was a problem hiding this comment.
Suggestion: The time.sleep(time_to_wait_before_reboot) call is duplicated in both the recover and revert methods of the DebianBasedSnapshot class. This could be refactored into a private method to avoid code duplication and improve maintainability. [maintainability]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| time.sleep(time_to_wait_before_reboot) | |
| if rebooter.is_reboot(self._device_reboot): | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() | |
| self._wait_and_reboot_if_needed(rebooter, time_to_wait_before_reboot) | |
| # Add this private method to the DebianBasedSnapshot class | |
| def _wait_and_reboot_if_needed(self, rebooter: Rebooter, time_to_wait: int) -> None: | |
| time.sleep(time_to_wait) | |
| if rebooter.is_reboot(self._device_reboot): | |
| logger.debug("Rebooting to recover from failed SOTA...") | |
| rebooter.reboot() |
| @@ -125,7 +125,7 @@ def __init__(self, | |||
| # If an exception occurs during string conversion, raise that exception | |||
| raise SotaError('package_list is not a string in manifest') from e | |||
|
|
|||
| self._device_reboot = parsed_manifest['deviceReboot'] | |||
| self._device_reboot = str(parsed_manifest['deviceReboot']) | |||
There was a problem hiding this comment.
Suggestion: Ensure that the deviceReboot key exists in parsed_manifest before attempting to access it to avoid a KeyError. [bug]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| self._device_reboot = str(parsed_manifest['deviceReboot']) | |
| self._device_reboot = str(parsed_manifest.get('deviceReboot', 'Yes')) |
| if self.sota_mode == 'download-only' or rebooter.is_reboot(self._device_reboot): | ||
| self._dispatcher_broker.telemetry("No reboot (SOTA pass)") | ||
| else: | ||
| self._dispatcher_broker.telemetry("Going to reboot (SOTA pass)") |
There was a problem hiding this comment.
Suggestion: Use a more robust check for reboot conditions that does not rely on string comparison, which can be error-prone and less maintainable. [enhancement]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| if self.sota_mode == 'download-only' or rebooter.is_reboot(self._device_reboot): | |
| self._dispatcher_broker.telemetry("No reboot (SOTA pass)") | |
| else: | |
| self._dispatcher_broker.telemetry("Going to reboot (SOTA pass)") | |
| if self.sota_mode == 'download-only' or not rebooter.is_reboot_required(): | |
| self._dispatcher_broker.telemetry("No reboot (SOTA pass)") | |
| else: | |
| self._dispatcher_broker.telemetry("Going to reboot (SOTA pass)") |
| if self.sota_mode == 'download-only' or rebooter.is_reboot(self._device_reboot): | ||
| self._dispatcher_broker.telemetry("No reboot (SOTA pass)") | ||
| else: | ||
| self._dispatcher_broker.telemetry("Going to reboot (SOTA pass)") |
There was a problem hiding this comment.
Suggestion: Refactor the conditional expression to improve readability and maintainability. [maintainability]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| if self.sota_mode == 'download-only' or rebooter.is_reboot(self._device_reboot): | |
| self._dispatcher_broker.telemetry("No reboot (SOTA pass)") | |
| else: | |
| self._dispatcher_broker.telemetry("Going to reboot (SOTA pass)") | |
| reboot_message = "No reboot (SOTA pass)" if self.sota_mode == 'download-only' or rebooter.is_reboot(self._device_reboot) else "Going to reboot (SOTA pass)" | |
| self._dispatcher_broker.telemetry(reboot_message) |
| assert type(SotaOsFactory(self.mock_disp_broker, None, []).get_os('Ubuntu').create_snapshotter('update', '1', False, 'Y')) \ | ||
| is DebianBasedSnapshot |
There was a problem hiding this comment.
Suggestion: Replace assert statements with unittest's assertIsInstance for better test messages and consistency. [best practice]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| assert type(SotaOsFactory(self.mock_disp_broker, None, []).get_os('Ubuntu').create_snapshotter('update', '1', False, 'Y')) \ | |
| is DebianBasedSnapshot | |
| ubuntu_os = SotaOsFactory(self.mock_disp_broker, None, []).get_os('Ubuntu') | |
| snapshotter = ubuntu_os.create_snapshotter('update', '1', False, 'Y') | |
| self.assertIsInstance(snapshotter, DebianBasedSnapshot) |
| assert type(SotaOsFactory(self.mock_disp_broker, None, []).get_os('YoctoX86_64').create_snapshotter('update', '1', False, 'Y')) \ | ||
| is YoctoSnapshot |
There was a problem hiding this comment.
Suggestion: Replace assert statements with unittest's assertIsInstance for better test messages and consistency. [best practice]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| assert type(SotaOsFactory(self.mock_disp_broker, None, []).get_os('YoctoX86_64').create_snapshotter('update', '1', False, 'Y')) \ | |
| is YoctoSnapshot | |
| yocto_os = SotaOsFactory(self.mock_disp_broker, None, []).get_os('YoctoX86_64') | |
| snapshotter = yocto_os.create_snapshotter('update', '1', False, 'Y') | |
| self.assertIsInstance(snapshotter, YoctoSnapshot) |
| assert type(SotaOsFactory(self.mock_disp_broker, None, []).get_os('Windows').create_snapshotter('update', '1', False, 'Y')) \ | ||
| is WindowsSnapshot |
There was a problem hiding this comment.
Suggestion: Replace assert statements with unittest's assertIsInstance for better test messages and consistency. [best practice]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| assert type(SotaOsFactory(self.mock_disp_broker, None, []).get_os('Windows').create_snapshotter('update', '1', False, 'Y')) \ | |
| is WindowsSnapshot | |
| windows_os = SotaOsFactory(self.mock_disp_broker, None, []).get_os('Windows') | |
| snapshotter = windows_os.create_snapshotter('update', '1', False, 'Y') | |
| self.assertIsInstance(snapshotter, WindowsSnapshot) |
| failed = False | ||
| try: | ||
| yocto_snapshot.take_snapshot() | ||
| except SotaError: |
There was a problem hiding this comment.
Suggestion: Use assertFalse to explicitly check for False conditions. [enhancement]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| failed = False | |
| try: | |
| yocto_snapshot.take_snapshot() | |
| except SotaError: | |
| with self.assertRaises(SotaError): | |
| yocto_snapshot.take_snapshot() |
| @@ -86,7 +86,7 @@ def test_take_snapshot_proceed_fail_publishes_error_succeeds(self, mock_state) - | |||
| trtl = Mock() | |||
| trtl.single_snapshot.return_value = "1", "Error!" | |||
|
|
|||
| ubuntu_snapshot = DebianBasedSnapshot(trtl, "command", dispatcher_broker, "1", True) | |||
| ubuntu_snapshot = DebianBasedSnapshot(trtl, "command", dispatcher_broker, "1", True, 'Y') | |||
| ubuntu_snapshot.take_snapshot() | |||
| assert dispatcher_broker.telemetry.call_count > 0 | |||
There was a problem hiding this comment.
Suggestion: Replace assert statements with self.assert* methods for better integration with unittest and more informative test failures. [best practice]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| assert dispatcher_broker.telemetry.call_count > 0 | |
| self.assertTrue(dispatcher_broker.telemetry.call_count > 0) |
| @@ -101,7 +101,7 @@ def test_take_snapshot_proceed_cannot_write_publishes_error_succeeds(self, mock_ | |||
| trtl = Mock() | |||
| trtl.single_snapshot.return_value = "1", None | |||
|
|
|||
| ubuntu_snapshot = DebianBasedSnapshot(trtl, "command", dispatcher_broker, "1", True) | |||
| ubuntu_snapshot = DebianBasedSnapshot(trtl, "command", dispatcher_broker, "1", True, 'Y') | |||
| ubuntu_snapshot.take_snapshot() | |||
|
|
|||
| assert dispatcher_broker.telemetry.call_count > 0 | |||
There was a problem hiding this comment.
Suggestion: Use assertIsNotNone instead of assert for checking if a value is not None for better test failure messages. [best practice]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| assert dispatcher_broker.telemetry.call_count > 0 | |
| self.assertTrue(dispatcher_broker.telemetry.call_count > 0) |
| @@ -113,7 +113,7 @@ def test_rollback_and_delete_snap_skipped_succeeds(self) -> None: | |||
| dispatcher_broker = Mock() | |||
| trtl = Mock() | |||
|
|
|||
| ubuntu_snapshot = DebianBasedSnapshot(trtl, "command", dispatcher_broker, "", True) | |||
| ubuntu_snapshot = DebianBasedSnapshot(trtl, "command", dispatcher_broker, "", True, "Y") | |||
| ubuntu_snapshot._rollback_and_delete_snap() | |||
|
|
|||
| assert dispatcher_broker.telemetry.call_count > 0 | |||
There was a problem hiding this comment.
Suggestion: Use assertTrue to check if a string is in the message for consistency with unittest practices. [best practice]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| assert dispatcher_broker.telemetry.call_count > 0 | |
| self.assertIn("skipped", message) |
| dispatcher_callbacks = Mock() | ||
| dispatcher_broker = Mock() |
There was a problem hiding this comment.
Suggestion: Remove unused 'dispatcher_callbacks' variable to clean up the code. [maintainability]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| dispatcher_callbacks = Mock() | |
| dispatcher_broker = Mock() | |
| dispatcher_broker = Mock() |
| dispatcher_callbacks = Mock() | ||
| dispatcher_broker = Mock() |
There was a problem hiding this comment.
Suggestion: Remove unused 'dispatcher_callbacks' variable to clean up the code. [maintainability]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| dispatcher_callbacks = Mock() | |
| dispatcher_broker = Mock() | |
| dispatcher_broker = Mock() |
| dispatcher_callbacks = Mock() | ||
| dispatcher_broker = Mock() |
There was a problem hiding this comment.
Suggestion: Remove unused 'dispatcher_callbacks' variable to clean up the code. [maintainability]
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| dispatcher_callbacks = Mock() | |
| dispatcher_broker = Mock() | |
| dispatcher_broker = Mock() |
The PR review is to check for sustainability and correctness. Sustainability is actually more business critical as correctness is largely tested into the code over time. Its useful to keep in mind that SW often outlives the HW it was written for and engineers move from job to job so it is critical that code developed for Intel be supportable across many years. It is up to the submitter and reviewer to look at the code from a perspective of what if we have to debug this 3 years from now after the author is no longer available and defect databases have been lost. Yes, that happens all the time when we are working with time scales of more than 2 years. When reviewing your code it is important to look at it from this perspective.
Author Mandatory (to be filled by PR Author/Submitter)
PULL DESCRIPTION
Provide a 1-2 line brief overview of the changes submitted through the Pull Request...
REFERENCES
Reference URL for issue tracking (JIRA/HSD/Github): <URL to be filled>
Note-1: Depending on complexity of code changes, use the suitable word for complexity: Low/Medium/High
Example: PR for Slim boot loader project with medium complexity can have the label as: ISDM_Medium
CODE MAINTAINABILITY
APPLICATION SPECIFIC
Maintainer Mandatory (to be filled by PR Reviewer/Approving Maintainer)
QUALITY CHECKS
CODE REVIEW IMPACT
Note P1/P2/P3/P4 denotes severity of defects found (Showstopper/High/Medium/Low) and xx denotes number of defects found
SECURITY CHECKS
Please check if your PR fulfills the following requirements:
Code must act as a teacher for future developers