-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(BREAKING CHANGE) RTC 536910 - [source] Remove ubuntuAptSource INBM configuration tag and underlying code; replaced with source command #472
Conversation
RTC 536910 Remove the UbuntuAptSource XML tag from the intel_manageability.conf file and its associated tag with it
/review |
RTC 536910 Remove the UbuntuAptSource XML tag from the intel_manageability.conf file and its associated tag with it
Code Review Analysis
Code Review Feedback💡 General suggestions: The PR successfully removes the ubuntuAptSource tag and associated code, which is a good practice for maintaining a clean and sustainable codebase. It is important to ensure that all references to the removed code are also cleaned up to prevent any potential issues in the future. Additionally, it would be beneficial to verify that the removal of this tag does not affect any other parts of the system that may have depended on it indirectly. ✨ Usage tips:
|
@tsirlapu I see that you've marked ubuntuAptSource as removed from the 4.2.0 version, but that's actually not true; 4.2.0 still has ubuntuAptSource. It would be better to simply say it's removed, period. as the person reading the docs would be reading them from the new version that has it removed, and not 4.2.0 or earlier. |
/generate_report |
Please provide the PR description rating here on a scale of 1 to 5 (1 being the lowest and 5 being the highest) -Summary Report of Code Review Evaluation metrics for the Repository:Code defects Acceptance ratio: 0/0Code improvements Acceptance ratio: 3/4 (75.0%)Code fixes Acceptance ratio: 0/0
|
RTC 536910
Remove the UbuntuAptSource XML tag from the intel_manageability.conf file and its associated tag with it
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