Implement using deb822 format in source application add command #457
Conversation
inbm/dispatcher-agent/tests/unit/source/test_ubuntu_source_cmd.py
Outdated
Show resolved
Hide resolved
nmgaston
force-pushed
the
source-app-add-deb822-format
branch
from
95a61a3 to
2f5b3ec
Compare
|
/review |
|
/review |
Code Review Analysis(review updated until commit 2f5b3ec)
Code Review Feedback💡 General suggestions: The PR introduces a significant enhancement to the 'source application add' command by supporting DEB822 format and making GPG key parameters optional. This is a welcome change that increases the flexibility of the command. However, it is crucial to ensure that all changes are backward compatible and that existing functionality is not broken. It is also important to verify that the new test cases cover all the new code paths introduced by these changes. ✨ Usage tips:
|
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | ||
| """Adds a source file and optional GPG key to be used during Ubuntu application updates.""" | ||
| # Step 1: Add key (Optional) | ||
| if parameters.gpg_key_name and parameters.gpg_key_uri: |
There was a problem hiding this comment.
Consider logging an informative message when GPG key parameters are not provided, for better traceability and debugging. [medium]
| except SourceError as err: | ||
| assert False, f"'UbuntuApplicationSourceManager.add' raised an exception {err}" | ||
|
|
||
| def test_add_app_deb_822_format_successfully(self): |
There was a problem hiding this comment.
Add a test case to verify the behavior when the GPG key is not provided during the add operation, ensuring the source is still added correctly. [important]
inbm/dispatcher-agent/fpm-template/usr/share/dispatcher-agent/manifest_schema.xsd
Show resolved
Hide resolved
| @@ -25,16 +26,16 @@ class SourceParameters: | |||
|
|
|||
| @dataclass(kw_only=True, frozen=True) | |||
| class ApplicationAddSourceParameters: | |||
There was a problem hiding this comment.
Ensure that the constants and types used throughout the source management commands are consistent with the new optional GPG key parameters. [medium]
| manifest += ('<gpg>' + '{0}' + '{1}' + '</gpg>').format(create_xml_tag(arguments, "uri"), | ||
| create_xml_tag(arguments, "keyname")) |
There was a problem hiding this comment.
Suggestion: Avoid using string concatenation for XML generation to prevent potential XML injection vulnerabilities. [security]
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.
| manifest += ('<gpg>' + '{0}' + '{1}' + '</gpg>').format(create_xml_tag(arguments, "uri"), | |
| create_xml_tag(arguments, "keyname")) | |
| manifest += f'<gpg>{create_xml_tag(arguments, "uri")}{create_xml_tag(arguments, "keyname")}</gpg>' |
| manifest = ('<?xml version="1.0" encoding="utf-8"?>' + | ||
| '<manifest><type>source</type>' + | ||
| '<applicationSource>' + | ||
| '<remove><gpg>' | ||
| f'{create_xml_tag(arguments, "keyname")}' + | ||
| '</gpg><repo>' + | ||
| f'{create_xml_tag(arguments, "filename")}' | ||
| '</repo>' | ||
| '</remove></applicationSource>' + | ||
| '</manifest>') | ||
| '<remove>') |
There was a problem hiding this comment.
Suggestion: Refactor the XML generation into a separate function to reduce 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.
| manifest = ('<?xml version="1.0" encoding="utf-8"?>' + | |
| '<manifest><type>source</type>' + | |
| '<applicationSource>' + | |
| '<remove><gpg>' | |
| f'{create_xml_tag(arguments, "keyname")}' + | |
| '</gpg><repo>' + | |
| f'{create_xml_tag(arguments, "filename")}' | |
| '</repo>' | |
| '</remove></applicationSource>' + | |
| '</manifest>') | |
| '<remove>') | |
| def generate_manifest(tag_name, content): | |
| return (f'<?xml version="1.0" encoding="utf-8"?>' | |
| f'<manifest><type>source</type>' | |
| f'<applicationSource>' | |
| f'<{tag_name}>{content}</{tag_name}></applicationSource>' | |
| f'</manifest>') | |
| # Usage example: | |
| manifest = generate_manifest('remove', content) |
| manifest = ('<?xml version="1.0" encoding="utf-8"?>' + | ||
| '<manifest>' + | ||
| '<type>source</type>' + | ||
| '<applicationSource>' + | ||
| '<add><gpg>' | ||
| '{0}' + | ||
| '{1}' | ||
| '</gpg><repo><repos>').format(create_xml_tag(arguments, "uri"), | ||
| create_xml_tag(arguments, "keyname")) | ||
| '<applicationSource>' + '<add>') |
There was a problem hiding this comment.
Suggestion: Replace the manual XML header creation with a constant to avoid repetition. [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.
| manifest = ('<?xml version="1.0" encoding="utf-8"?>' + | |
| '<manifest>' + | |
| '<type>source</type>' + | |
| '<applicationSource>' + | |
| '<add><gpg>' | |
| '{0}' + | |
| '{1}' | |
| '</gpg><repo><repos>').format(create_xml_tag(arguments, "uri"), | |
| create_xml_tag(arguments, "keyname")) | |
| '<applicationSource>' + '<add>') | |
| XML_HEADER = '<?xml version="1.0" encoding="utf-8"?>' | |
| manifest = (f'{XML_HEADER}' | |
| '<manifest><type>source</type>' | |
| '<applicationSource><add>') |
| for source in args.sources: | ||
| manifest += '<source_pkg>' + source.strip() + '</source_pkg>' | ||
| manifest += '<source_pkg>' + source + '</source_pkg>' |
There was a problem hiding this comment.
Suggestion: Use a list and join to build the XML string to improve performance. [performance]
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.
| for source in args.sources: | |
| manifest += '<source_pkg>' + source.strip() + '</source_pkg>' | |
| manifest += '<source_pkg>' + source + '</source_pkg>' | |
| sources_xml = ''.join(f'<source_pkg>{source}</source_pkg>' for source in args.sources) | |
| manifest += f'<repo><repos>{sources_xml}</repos>' |
| expected = '<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | ||
| '<add><repo><repos>' \ | ||
| '<source_pkg>deb http://example.com/ focal main restricted universe</source_pkg>' \ | ||
| '<source_pkg>deb-src http://example.com/ focal-security main</source_pkg>' \ | ||
| '</repos><filename>intel-gpu-jammy.list</filename></repo></add></applicationSource></manifest>' |
There was a problem hiding this comment.
Suggestion: Avoid using hardcoded XML strings in tests. Use an XML library to create and compare XML documents, which will make the tests less brittle to changes in the XML structure. [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.
| expected = '<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | |
| '<add><repo><repos>' \ | |
| '<source_pkg>deb http://example.com/ focal main restricted universe</source_pkg>' \ | |
| '<source_pkg>deb-src http://example.com/ focal-security main</source_pkg>' \ | |
| '</repos><filename>intel-gpu-jammy.list</filename></repo></add></applicationSource></manifest>' | |
| from xml.etree.ElementTree import Element, SubElement, tostring | |
| def construct_manifest_xml(sources, filename): | |
| manifest = Element('manifest') | |
| manifest.set('version', '1.0') | |
| manifest.set('encoding', 'utf-8') | |
| application_source = SubElement(manifest, 'applicationSource') | |
| add = SubElement(application_source, 'add') | |
| repo = SubElement(add, 'repo') | |
| repos = SubElement(repo, 'repos') | |
| for source in sources: | |
| source_pkg = SubElement(repos, 'source_pkg') | |
| source_pkg.text = source | |
| filename_element = SubElement(repo, 'filename') | |
| filename_element.text = filename | |
| return tostring(manifest, encoding='utf-8').decode() | |
| sources = [ | |
| 'deb http://example.com/ focal main restricted universe', | |
| 'deb-src http://example.com/ focal-security main' | |
| ] | |
| expected = construct_manifest_xml(sources, 'intel-gpu-jammy.list') |
| f = self.arg_parser.parse_args( | ||
| ['source', 'application', 'add', | ||
| '--sources', 'X-Repolib-Name: Google Chrome', | ||
| 'Enabled: yes', | ||
| 'Types: deb', | ||
| 'URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub', | ||
| 'Suites: stable', | ||
| 'Components: main', | ||
| '--filename', 'intel-gpu-jammy.list']) |
There was a problem hiding this comment.
Suggestion: Consider using a loop to iterate over the 'sources' list to avoid code repetition 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.
| f = self.arg_parser.parse_args( | |
| ['source', 'application', 'add', | |
| '--sources', 'X-Repolib-Name: Google Chrome', | |
| 'Enabled: yes', | |
| 'Types: deb', | |
| 'URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub', | |
| 'Suites: stable', | |
| 'Components: main', | |
| '--filename', 'intel-gpu-jammy.list']) | |
| sources_list = [ | |
| 'X-Repolib-Name: Google Chrome', | |
| 'Enabled: yes', | |
| 'Types: deb', | |
| 'URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub', | |
| 'Suites: stable', | |
| 'Components: main' | |
| ] | |
| f = self.arg_parser.parse_args( | |
| ['source', 'application', 'add', | |
| '--sources'] + sources_list + | |
| ['--filename', 'intel-gpu-jammy.list']) |
| expected = '<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | ||
| '<add><repo><repos><source_pkg>X-Repolib-Name: Google Chrome</source_pkg>' \ | ||
| '<source_pkg>Enabled: yes</source_pkg>' \ | ||
| '<source_pkg>Types: deb</source_pkg>'\ | ||
| '<source_pkg>URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub</source_pkg>' \ | ||
| '<source_pkg>Suites: stable</source_pkg>' \ | ||
| '<source_pkg>Components: main</source_pkg>' \ | ||
| '</repos><filename>google-chrome.sources</filename></repo></add></applicationSource></manifest>' |
There was a problem hiding this comment.
Suggestion: Refactor the repeated XML manifest string construction into a helper method to avoid duplication and improve code readability. [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.
| expected = '<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | |
| '<add><repo><repos><source_pkg>X-Repolib-Name: Google Chrome</source_pkg>' \ | |
| '<source_pkg>Enabled: yes</source_pkg>' \ | |
| '<source_pkg>Types: deb</source_pkg>'\ | |
| '<source_pkg>URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub</source_pkg>' \ | |
| '<source_pkg>Suites: stable</source_pkg>' \ | |
| '<source_pkg>Components: main</source_pkg>' \ | |
| '</repos><filename>google-chrome.sources</filename></repo></add></applicationSource></manifest>' | |
| def construct_manifest(source_packages, filename): | |
| source_pkg_elements = ''.join(f'<source_pkg>{pkg}</source_pkg>' for pkg in source_packages) | |
| return f'<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | |
| f'<add><repo><repos>{source_pkg_elements}</repos><filename>{filename}</filename></repo></add></applicationSource></manifest>' | |
| source_packages = [ | |
| 'X-Repolib-Name: Google Chrome', | |
| 'Enabled: yes', | |
| 'Types: deb', | |
| 'URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub', | |
| 'Suites: stable', | |
| 'Components: main' | |
| ] | |
| expected = construct_manifest(source_packages, 'google-chrome.sources') |
| file_name: str | ||
| sources: list[str] = field(default_factory=lambda: []) | ||
| gpg_key_uri: Optional[str] = field(default=None) | ||
| gpg_key_name: Optional[str] = field(default=None) |
There was a problem hiding this comment.
Suggestion: Ensure that 'sources' list is validated to contain valid APT source entries to prevent potential issues with malformed sources. [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.
| file_name: str | |
| sources: list[str] = field(default_factory=lambda: []) | |
| gpg_key_uri: Optional[str] = field(default=None) | |
| gpg_key_name: Optional[str] = field(default=None) | |
| @dataclass(kw_only=True, frozen=True) | |
| class ApplicationAddSourceParameters: | |
| file_name: str | |
| sources: list[str] = field(default_factory=list) | |
| gpg_key_uri: Optional[str] = field(default=None) | |
| gpg_key_name: Optional[str] = field(default=None) | |
| def __post_init__(self): | |
| for source in self.sources: | |
| validate_apt_source(source) |
| try: | ||
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | ||
| except XmlException: | ||
| # These children may not be present | ||
| logger.info(f"Optional GPG key parameters not present in manifest") | ||
| filename = parsed_head.get_children("applicationSource/remove/repo")["filename"] | ||
| application_source_manager.remove( | ||
| ApplicationRemoveSourceParameters(file_name=filename, gpg_key_name=keyname) | ||
| ) | ||
| return Result(status=200, message="SUCCESS") | ||
|
|
||
| if "add" in app_action: | ||
| gpg_key_uri = parsed_head.get_children("applicationSource/add/gpg")["uri"] | ||
| gpg_key_name = parsed_head.get_children("applicationSource/add/gpg")["keyname"] | ||
| gpg_key_uri = None | ||
| gpg_key_name = None | ||
|
|
||
| try: | ||
| gpg_key_uri = parsed_head.get_children("applicationSource/add/gpg")["uri"] | ||
| gpg_key_name = parsed_head.get_children("applicationSource/add/gpg")["keyname"] | ||
| except XmlException: | ||
| # These children may not be present | ||
| logger.info(f"Optional GPG key parameters not present in manifest") |
There was a problem hiding this comment.
Suggestion: Refactor the repeated try-except blocks for GPG key parameters into a separate function to improve code 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.
| try: | |
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | |
| except XmlException: | |
| # These children may not be present | |
| logger.info(f"Optional GPG key parameters not present in manifest") | |
| filename = parsed_head.get_children("applicationSource/remove/repo")["filename"] | |
| application_source_manager.remove( | |
| ApplicationRemoveSourceParameters(file_name=filename, gpg_key_name=keyname) | |
| ) | |
| return Result(status=200, message="SUCCESS") | |
| if "add" in app_action: | |
| gpg_key_uri = parsed_head.get_children("applicationSource/add/gpg")["uri"] | |
| gpg_key_name = parsed_head.get_children("applicationSource/add/gpg")["keyname"] | |
| gpg_key_uri = None | |
| gpg_key_name = None | |
| try: | |
| gpg_key_uri = parsed_head.get_children("applicationSource/add/gpg")["uri"] | |
| gpg_key_name = parsed_head.get_children("applicationSource/add/gpg")["keyname"] | |
| except XmlException: | |
| # These children may not be present | |
| logger.info(f"Optional GPG key parameters not present in manifest") | |
| def get_optional_gpg_keys(parsed_head: XmlHandler, action: str) -> tuple: | |
| gpg_key_uri = None | |
| gpg_key_name = None | |
| try: | |
| gpg_keys = parsed_head.get_children(f"applicationSource/{action}/gpg") | |
| gpg_key_uri = gpg_keys.get("uri") | |
| gpg_key_name = gpg_keys.get("keyname") | |
| except XmlException: | |
| logger.info(f"Optional GPG key parameters not present in manifest for {action}") | |
| return gpg_key_uri, gpg_key_name | |
| # Usage in "remove" action | |
| keyname = get_optional_gpg_keys(parsed_head, "remove")[1] | |
| # Usage in "add" action | |
| gpg_key_uri, gpg_key_name = get_optional_gpg_keys(parsed_head, "add") |
| if parameters.gpg_key_name and parameters.gpg_key_uri: | ||
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) |
There was a problem hiding this comment.
Suggestion: Consider validating the GPG key URI before attempting to add it. This can prevent potential security risks associated with malformed or malicious URIs. [security]
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 parameters.gpg_key_name and parameters.gpg_key_uri: | |
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | |
| if parameters.gpg_key_name and parameters.gpg_key_uri: | |
| if validate_gpg_key_uri(parameters.gpg_key_uri): | |
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | |
| else: | |
| raise SourceError(f"Invalid GPG key URI: {parameters.gpg_key_uri}") |
| if parameters.gpg_key_name: | ||
| # Remove the GPG key (Optional) | ||
| remove_gpg_key_if_exists(parameters.gpg_key_name) |
There was a problem hiding this comment.
Suggestion: To ensure that the file name does not contain any directory traversal characters or sequences, validate parameters.file_name before using it to construct the file path. [security]
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 parameters.gpg_key_name: | |
| # Remove the GPG key (Optional) | |
| remove_gpg_key_if_exists(parameters.gpg_key_name) | |
| if parameters.gpg_key_name: | |
| remove_gpg_key_if_exists(parameters.gpg_key_name) | |
| if not is_valid_filename(parameters.file_name): | |
| raise SourceError(f"Invalid file name: {parameters.file_name}") |
| remove_gpg_key_if_exists(parameters.gpg_key_name) | ||
| if parameters.gpg_key_name: | ||
| # Remove the GPG key (Optional) | ||
| remove_gpg_key_if_exists(parameters.gpg_key_name) | ||
|
|
||
| # Remove the file under /etc/apt/sources.list.d | ||
| try: |
There was a problem hiding this comment.
Suggestion: To avoid potential race conditions when checking for file existence and then removing it, use a try-except block to handle the removal operation. [robustness]
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.
| try: | |
| try: | |
| remove_file( | |
| get_canonical_representation_of_path( | |
| os.path.join(UBUNTU_APT_SOURCES_LIST_D, parameters.file_name) | |
| ) | |
| ) | |
| except OSError as e: | |
| raise SourceError(f"Error removing file: {parameters.file_name}: {e}") |
| """Adds a source file and optional GPG key to be used during Ubuntu application updates.""" | ||
| # Step 1: Add key (Optional) | ||
| if parameters.gpg_key_name and parameters.gpg_key_uri: | ||
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | ||
|
|
||
| # Step 2: Add the source | ||
| try: |
There was a problem hiding this comment.
Suggestion: Use a context manager when working with files to ensure that the file is properly closed even if an error occurs. [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.
| try: | |
| with open(os.path.join(UBUNTU_APT_SOURCES_LIST_D, parameters.file_name), 'w') as file: | |
| file.writelines(f"{source}\n" for source in parameters.sources) |
| if parameters.gpg_key_name and parameters.gpg_key_uri: | ||
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | ||
|
|
||
| # Step 2: Add the source | ||
| try: |
There was a problem hiding this comment.
Suggestion: To improve the readability and maintainability of the code, consider refactoring the add method to separate the concerns of adding a GPG key and creating a source file. [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 parameters.gpg_key_name and parameters.gpg_key_uri: | |
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | |
| # Step 2: Add the source | |
| try: | |
| def add_gpg_key_if_provided(self, parameters: ApplicationAddSourceParameters) -> None: | |
| if parameters.gpg_key_name and parameters.gpg_key_uri: | |
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | |
| def create_source_file(self, parameters: ApplicationAddSourceParameters) -> None: | |
| create_file_with_contents( | |
| os.path.join(UBUNTU_APT_SOURCES_LIST_D, parameters.file_name), parameters.sources | |
| ) | |
| def add(self, parameters: ApplicationAddSourceParameters) -> None: | |
| self.add_gpg_key_if_provided(parameters) | |
| self.create_source_file(parameters) |
| with patch("builtins.open", new_callable=mock_open()): | ||
| command.update(params) |
There was a problem hiding this comment.
Suggestion: The 'test_update_app_source_successfully' method is missing an assertion to verify the expected behavior. Tests should always include assertions to validate the test's intent. [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.
| with patch("builtins.open", new_callable=mock_open()): | |
| command.update(params) | |
| with patch("builtins.open", new_callable=mock_open()) as m: | |
| command.update(params) | |
| m.assert_called_with('expected_file_path', 'w') | |
| m().write.assert_called_with('expected_content') |
| except SourceError as err: | ||
| assert False, f"'UbuntuApplicationSourceManager.add' raised an exception {err}" |
There was a problem hiding this comment.
Suggestion: The 'test_add_app_with_gpg_key_successfully' and 'test_add_app_deb_822_format_successfully' methods are catching a broad 'SourceError' exception and asserting False, which is not a common practice in test cases. Instead, use 'pytest.fail' to provide a clearer message on test failure. [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.
| except SourceError as err: | |
| assert False, f"'UbuntuApplicationSourceManager.add' raised an exception {err}" | |
| except SourceError as err: | |
| pytest.fail(f"'UbuntuApplicationSourceManager.add' raised an exception {err}") |
|
Persistent review updated to latest commit 2f5b3ec |
|
|
||
|
|
||
| class TestSourceApplicationParser(TestCase): | ||
| def setUp(self): | ||
| self.arg_parser = ArgsParser() | ||
| self.maxDiff = None | ||
|
|
||
| def test_parse_add_arguments_successfully(self): | ||
| def test_parse_add_all_arguments_successfully(self): |
There was a problem hiding this comment.
Add more test cases to cover scenarios where the --sources argument is provided in an incorrect DEB822 format. [important]
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | ||
| """Adds a source file and optional GPG key to be used during Ubuntu application updates.""" | ||
| # Step 1: Add key (Optional) | ||
| if parameters.gpg_key_name and parameters.gpg_key_uri: |
There was a problem hiding this comment.
Ensure that the add method handles the case where the DEB822 format is used without a GPG key. [important]
| @@ -192,14 +193,44 @@ def test_update_sources_os_error(self): | |||
|
|
|||
|
|
|||
| class TestUbuntuApplicationSourceManager: | |||
| @patch("dispatcher.source.ubuntu_source_manager.move_file") | |||
| def test_update_app_source_successfully(self, mock_move): | |||
| def test_add_app_with_gpg_key_successfully(self): | |||
There was a problem hiding this comment.
Add unit tests to verify the behavior of the UbuntuApplicationSourceManager.add method when DEB822 format is used. [important]
| @@ -333,7 +333,7 @@ | |||
| <xs:element name="repos" minOccurs="1" maxOccurs="1"> | |||
| <xs:complexType> | |||
| <xs:sequence> | |||
| <xs:element name="source_pkg" type="Max200Chars" minOccurs="1" maxOccurs="2"/> | |||
| <xs:element name="source_pkg" type="Max200Chars" minOccurs="1" maxOccurs="45"/> | |||
There was a problem hiding this comment.
Update the XML schema to allow for a variable number of source_pkg elements when using DEB822 format. [important]
| app_add_parser.add_argument('--gpgKeyUri', '-gku', required=False, | ||
| type=lambda x: validate_string_less_than_n_characters( | ||
| x, 'str', 1500), | ||
| help='Uri from which to download GPG key') | ||
| app_add_parser.add_argument('--gpgKeyName', '-gkn', required=True, | ||
| app_add_parser.add_argument('--gpgKeyName', '-gkn', required=False, | ||
| type=lambda x: validate_string_less_than_n_characters( | ||
| x, 'str', 200), | ||
| help='Name to store the GPG key information') |
There was a problem hiding this comment.
Suggestion: The 'required' flag for '--gpgKeyUri' and '--gpgKeyName' arguments in the 'app_add_parser' has been changed from True to False. Ensure that the rest of the code properly handles cases where these arguments might not be provided, to prevent runtime errors. [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.
| app_add_parser.add_argument('--gpgKeyUri', '-gku', required=False, | |
| type=lambda x: validate_string_less_than_n_characters( | |
| x, 'str', 1500), | |
| help='Uri from which to download GPG key') | |
| app_add_parser.add_argument('--gpgKeyName', '-gkn', required=True, | |
| app_add_parser.add_argument('--gpgKeyName', '-gkn', required=False, | |
| type=lambda x: validate_string_less_than_n_characters( | |
| x, 'str', 200), | |
| help='Name to store the GPG key information') | |
| # It's assumed that the application_add function is designed to handle optional GPG key parameters. | |
| # If not, the function should be updated accordingly to avoid runtime errors when these arguments are not provided. |
| for source in args.sources: | ||
| manifest += '<source_pkg>' + source.strip() + '</source_pkg>' | ||
| manifest += '<source_pkg>' + source + '</source_pkg>' |
There was a problem hiding this comment.
Suggestion: Ensure that the source variable is stripped of leading and trailing whitespace before being added to the XML manifest. [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.
| for source in args.sources: | |
| manifest += '<source_pkg>' + source.strip() + '</source_pkg>' | |
| manifest += '<source_pkg>' + source + '</source_pkg>' | |
| for source in args.sources: | |
| manifest += f'<source_pkg>{source.strip()}</source_pkg>' |
| f'{create_xml_tag(arguments, "filename")}' | ||
| '</repo>' | ||
| '</remove></applicationSource>' + | ||
| '</manifest>') | ||
|
|
||
| print(f"manifest {manifest}") |
There was a problem hiding this comment.
Suggestion: Use a context manager to handle the printing of the manifest to ensure that resources are properly managed. [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.
| print(f"manifest {manifest}") | |
| with open('manifest_log.txt', 'a') as log_file: | |
| log_file.write(f"manifest {manifest}\n") |
| manifest += ('<gpg>' + '{0}' + '{1}' + '</gpg>').format(create_xml_tag(arguments, "uri"), | ||
| create_xml_tag(arguments, "keyname")) |
There was a problem hiding this comment.
Suggestion: Replace string concatenation with f-strings for better readability and performance. [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.
| manifest += ('<gpg>' + '{0}' + '{1}' + '</gpg>').format(create_xml_tag(arguments, "uri"), | |
| create_xml_tag(arguments, "keyname")) | |
| manifest += f'<gpg>{create_xml_tag(arguments, "uri")}{create_xml_tag(arguments, "keyname")}</gpg>' |
| manifest += ('</repos>' | ||
| f'{create_xml_tag(arguments, "filename")}</repo>' |
There was a problem hiding this comment.
Suggestion: Remove unnecessary parentheses from the single-line f-string. [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.
| manifest += ('</repos>' | |
| f'{create_xml_tag(arguments, "filename")}</repo>' | |
| manifest += f'</repos>{create_xml_tag(arguments, "filename")}</repo></add></applicationSource></manifest>' |
| '<remove>') | ||
|
|
||
| if args.gpgKeyName: | ||
| manifest += f'<gpg>{create_xml_tag(arguments, "keyname")}</gpg>' |
There was a problem hiding this comment.
Suggestion: Avoid using f-strings inside expressions where curly braces are used for XML tags, as it might cause confusion. Use format method or concatenate the strings for clarity. [readability]
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.
| manifest += f'<gpg>{create_xml_tag(arguments, "keyname")}</gpg>' | |
| manifest += '<gpg>' + create_xml_tag(arguments, "keyname") + '</gpg>' |
| @patch('inbm_lib.mqttclient.mqtt.mqtt.Client.connect') | ||
| def test_raise_application_add_with_only_one_gpgKeyUri_param(self, m_connect): | ||
| p = self.arg_parser.parse_args( | ||
| ['source', 'application', 'add', | ||
| '--gpgKeyUri', 'https://repositories.intel.com/gpu/intel-graphics.key', | ||
| '--sources', 'deb http://example.com/ focal main restricted universe', | ||
| 'deb-src http://example.com/ focal-security main', | ||
| '--filename', 'intel-gpu-jammy.list']) | ||
| with pytest.raises(InbcException, | ||
| match="Source requires either both gpgKeyUri and gpgKeyName " | ||
| "to be provided, or neither of them."): | ||
| Inbc(p, 'source', False) | ||
|
|
||
| @patch('inbm_lib.mqttclient.mqtt.mqtt.Client.connect') | ||
| def test_raise_application_add_with_only_one_gpgKeyName_param(self, m_connect): | ||
| p = self.arg_parser.parse_args( | ||
| ['source', 'application', 'add', | ||
| '--gpgKeyName', 'intel-graphics.gpg', | ||
| '--sources', 'deb http://example.com/ focal main restricted universe', | ||
| 'deb-src http://example.com/ focal-security main', | ||
| '--filename', 'intel-gpu-jammy.list']) | ||
| with pytest.raises(InbcException, | ||
| match="Source requires either both gpgKeyUri and gpgKeyName " | ||
| "to be provided, or neither of them."): | ||
| Inbc(p, 'source', False) |
There was a problem hiding this comment.
Suggestion: Use a data-driven approach for tests to reduce code duplication and improve maintainability. [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.
| @patch('inbm_lib.mqttclient.mqtt.mqtt.Client.connect') | |
| def test_raise_application_add_with_only_one_gpgKeyUri_param(self, m_connect): | |
| p = self.arg_parser.parse_args( | |
| ['source', 'application', 'add', | |
| '--gpgKeyUri', 'https://repositories.intel.com/gpu/intel-graphics.key', | |
| '--sources', 'deb http://example.com/ focal main restricted universe', | |
| 'deb-src http://example.com/ focal-security main', | |
| '--filename', 'intel-gpu-jammy.list']) | |
| with pytest.raises(InbcException, | |
| match="Source requires either both gpgKeyUri and gpgKeyName " | |
| "to be provided, or neither of them."): | |
| Inbc(p, 'source', False) | |
| @patch('inbm_lib.mqttclient.mqtt.mqtt.Client.connect') | |
| def test_raise_application_add_with_only_one_gpgKeyName_param(self, m_connect): | |
| p = self.arg_parser.parse_args( | |
| ['source', 'application', 'add', | |
| '--gpgKeyName', 'intel-graphics.gpg', | |
| '--sources', 'deb http://example.com/ focal main restricted universe', | |
| 'deb-src http://example.com/ focal-security main', | |
| '--filename', 'intel-gpu-jammy.list']) | |
| with pytest.raises(InbcException, | |
| match="Source requires either both gpgKeyUri and gpgKeyName " | |
| "to be provided, or neither of them."): | |
| Inbc(p, 'source', False) | |
| @patch('inbm_lib.mqttclient.mqtt.mqtt.Client.connect') | |
| def test_raise_application_add_with_missing_gpg_params(self, m_connect): | |
| test_cases = [ | |
| (['--gpgKeyUri', 'https://repositories.intel.com/gpu/intel-graphics.key'], 'gpgKeyName'), | |
| (['--gpgKeyName', 'intel-graphics.gpg'], 'gpgKeyUri') | |
| ] | |
| for args, missing_param in test_cases: | |
| with self.subTest(missing_param=missing_param): | |
| p = self.arg_parser.parse_args(['source', 'application', 'add'] + args + [ | |
| '--sources', 'deb http://example.com/ focal main restricted universe', | |
| 'deb-src http://example.com/ focal-security main', | |
| '--filename', 'intel-gpu-jammy.list']) | |
| with pytest.raises(InbcException, | |
| match=f"Source requires either both gpgKeyUri and {missing_param} " | |
| "to be provided, or neither of them."): | |
| Inbc(p, 'source', False) |
| 'deb-src http://example.com/ focal-security main']) | ||
| self.assertEqual(f.filename, 'intel-gpu-jammy.list') |
There was a problem hiding this comment.
Suggestion: Consider using a loop to avoid code duplication when asserting multiple attributes of the 'f' object. [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.
| 'deb-src http://example.com/ focal-security main']) | |
| self.assertEqual(f.filename, 'intel-gpu-jammy.list') | |
| expected_values = { | |
| 'gpgKeyUri': 'https://repositories.intel.com/gpu/intel-graphics.key', | |
| 'gpgKeyName': 'intel-graphics.gpg', | |
| 'sources': ['deb http://example.com/ focal main restricted universe', | |
| 'deb-src http://example.com/ focal-security main'], | |
| 'filename': 'intel-gpu-jammy.list' | |
| } | |
| for attr, expected in expected_values.items(): | |
| self.assertEqual(getattr(f, attr), expected) |
| expected = '<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | ||
| '<add><repo><repos><source_pkg>X-Repolib-Name: Google Chrome</source_pkg>' \ | ||
| '<source_pkg>Enabled: yes</source_pkg>' \ | ||
| '<source_pkg>Types: deb</source_pkg>'\ | ||
| '<source_pkg>URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub</source_pkg>' \ | ||
| '<source_pkg>Suites: stable</source_pkg>' \ | ||
| '<source_pkg>Components: main</source_pkg>' \ | ||
| '</repos><filename>google-chrome.sources</filename></repo></add></applicationSource></manifest>' |
There was a problem hiding this comment.
Suggestion: Extract the XML manifest string construction into a separate method 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.
| expected = '<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>' \ | |
| '<add><repo><repos><source_pkg>X-Repolib-Name: Google Chrome</source_pkg>' \ | |
| '<source_pkg>Enabled: yes</source_pkg>' \ | |
| '<source_pkg>Types: deb</source_pkg>'\ | |
| '<source_pkg>URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub</source_pkg>' \ | |
| '<source_pkg>Suites: stable</source_pkg>' \ | |
| '<source_pkg>Components: main</source_pkg>' \ | |
| '</repos><filename>google-chrome.sources</filename></repo></add></applicationSource></manifest>' | |
| def construct_manifest_xml(repo_details, filename): | |
| xml_parts = ['<?xml version="1.0" encoding="utf-8"?><manifest><type>source</type><applicationSource>', | |
| '<add><repo><repos>'] | |
| for detail in repo_details: | |
| xml_parts.append(f'<source_pkg>{detail}</source_pkg>') | |
| xml_parts.append(f'</repos><filename>{filename}</filename></repo></add></applicationSource></manifest>') | |
| return ''.join(xml_parts) | |
| expected = construct_manifest_xml([ | |
| 'X-Repolib-Name: Google Chrome', | |
| 'Enabled: yes', | |
| 'Types: deb', | |
| 'URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub', | |
| 'Suites: stable', | |
| 'Components: main' | |
| ], 'google-chrome.sources') |
| f = self.arg_parser.parse_args( | ||
| ['source', 'application', 'add', | ||
| '--gpgKeyUri', 'https://repositories.intel.com/gpu/intel-graphics.key', |
There was a problem hiding this comment.
Suggestion: Refactor the test cases to use a helper method for parsing arguments to reduce redundancy. [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.
| f = self.arg_parser.parse_args( | |
| ['source', 'application', 'add', | |
| '--gpgKeyUri', 'https://repositories.intel.com/gpu/intel-graphics.key', | |
| def parse_add_arguments(self, args): | |
| return self.arg_parser.parse_args(['source', 'application', 'add'] + args) | |
| f = self.parse_add_arguments([ | |
| '--gpgKeyUri', 'https://repositories.intel.com/gpu/intel-graphics.key', | |
| '--gpgKeyName', 'intel-graphics.gpg', | |
| '--sources', 'deb http://example.com/ focal main restricted universe', | |
| 'deb-src http://example.com/ focal-security main', | |
| '--filename', 'intel-gpu-jammy.list' | |
| ]) |
| @@ -25,16 +26,16 @@ class SourceParameters: | |||
|
|
|||
| @dataclass(kw_only=True, frozen=True) | |||
| class ApplicationAddSourceParameters: | |||
| gpg_key_uri: str | |||
| gpg_key_name: str | |||
| file_name: str | |||
There was a problem hiding this comment.
Suggestion: Validate the file_name to ensure it ends with '.list' since it is a convention for APT source list files. [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.
| file_name: str | |
| @dataclass(kw_only=True, frozen=True) | |
| class ApplicationAddSourceParameters: | |
| file_name: str = field(init=False) | |
| source_list_file_name: str = field(default=None) | |
| def __post_init__(self): | |
| if not self.source_list_file_name.endswith('.list'): | |
| raise ValueError("file_name must end with '.list'") | |
| self.file_name = self.source_list_file_name |
| class ApplicationAddSourceParameters: | ||
| gpg_key_uri: str | ||
| gpg_key_name: str | ||
| file_name: str | ||
| sources: list[str] = field(default_factory=lambda: []) | ||
| gpg_key_uri: Optional[str] = field(default=None) | ||
| gpg_key_name: Optional[str] = field(default=None) |
There was a problem hiding this comment.
Suggestion: Consider using a more specific type than list for sources to ensure immutability, such as tuple. [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.
| class ApplicationAddSourceParameters: | |
| gpg_key_uri: str | |
| gpg_key_name: str | |
| file_name: str | |
| sources: list[str] = field(default_factory=lambda: []) | |
| gpg_key_uri: Optional[str] = field(default=None) | |
| gpg_key_name: Optional[str] = field(default=None) | |
| @dataclass(kw_only=True, frozen=True) | |
| class ApplicationAddSourceParameters: | |
| file_name: str | |
| sources: tuple[str, ...] = field(default_factory=tuple) | |
| gpg_key_uri: Optional[str] = field(default=None) | |
| gpg_key_name: Optional[str] = field(default=None) |
| @dataclass(kw_only=True, frozen=True) | ||
| class ApplicationRemoveSourceParameters: | ||
| gpg_key_name: str | ||
| file_name: str | ||
| gpg_key_name: Optional[str] = field(default=None) |
There was a problem hiding this comment.
Suggestion: Since the class is frozen, consider using tuple instead of list for the sources field to ensure all elements are immutable. [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.
| @dataclass(kw_only=True, frozen=True) | |
| class ApplicationRemoveSourceParameters: | |
| gpg_key_name: str | |
| file_name: str | |
| gpg_key_name: Optional[str] = field(default=None) | |
| @dataclass(kw_only=True, frozen=True) | |
| class ApplicationRemoveSourceParameters: | |
| source_list_file_name: str | |
| gpg_key_name: Optional[str] = field(default=None) |
| if "remove" in app_action: | ||
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | ||
| keyname = None | ||
| try: | ||
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | ||
| except XmlException: | ||
| # These children may not be present | ||
| logger.info(f"Optional GPG key parameters not present in manifest") | ||
| filename = parsed_head.get_children("applicationSource/remove/repo")["filename"] |
There was a problem hiding this comment.
Suggestion: Refactor the repeated try-except blocks for GPG key retrieval into a separate function to reduce code duplication. [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 "remove" in app_action: | |
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | |
| keyname = None | |
| try: | |
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | |
| except XmlException: | |
| # These children may not be present | |
| logger.info(f"Optional GPG key parameters not present in manifest") | |
| filename = parsed_head.get_children("applicationSource/remove/repo")["filename"] | |
| if "remove" in app_action: | |
| keyname = get_optional_gpg_key(parsed_head, "applicationSource/remove/gpg") | |
| filename = parsed_head.get_children("applicationSource/remove/repo")["filename"] | |
| application_source_manager.remove( | |
| ApplicationRemoveSourceParameters(file_name=filename, gpg_key_name=keyname) | |
| ) | |
| return Result(status=200, message="SUCCESS") | |
| def get_optional_gpg_key(parsed_head: XmlHandler, path: str) -> Optional[str]: | |
| try: | |
| return parsed_head.get_children(path)["keyname"] | |
| except XmlException: | |
| logger.info(f"Optional GPG key parameters not present in manifest") | |
| return None |
| keyname = None | ||
| try: | ||
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | ||
| except XmlException: | ||
| # These children may not be present | ||
| logger.info(f"Optional GPG key parameters not present in manifest") |
There was a problem hiding this comment.
Suggestion: Use a context manager to ensure that the logger's context is properly set during the execution of the function. [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.
| keyname = None | |
| try: | |
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | |
| except XmlException: | |
| # These children may not be present | |
| logger.info(f"Optional GPG key parameters not present in manifest") | |
| if "remove" in app_action: | |
| with logger.contextualize(action="remove", component="applicationSource"): | |
| keyname = None | |
| try: | |
| keyname = parsed_head.get_children("applicationSource/remove/gpg")["keyname"] | |
| except XmlException: | |
| # These children may not be present | |
| logger.info("Optional GPG key parameters not present in manifest") |
| if parameters.gpg_key_name and parameters.gpg_key_uri: | ||
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) |
There was a problem hiding this comment.
Suggestion: Validate the gpg_key_uri to ensure it's a well-formed URL before attempting to add the GPG key. [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 parameters.gpg_key_name and parameters.gpg_key_uri: | |
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) | |
| if parameters.gpg_key_name and parameters.gpg_key_uri: | |
| if not is_valid_url(parameters.gpg_key_uri): | |
| raise SourceError(f"Invalid GPG key URI: {parameters.gpg_key_uri}") | |
| add_gpg_key(parameters.gpg_key_uri, parameters.gpg_key_name) |
| if parameters.gpg_key_name: | ||
| # Remove the GPG key (Optional) | ||
| remove_gpg_key_if_exists(parameters.gpg_key_name) |
There was a problem hiding this comment.
Suggestion: Refactor the code to avoid duplication of the GPG key removal logic. [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 parameters.gpg_key_name: | |
| # Remove the GPG key (Optional) | |
| remove_gpg_key_if_exists(parameters.gpg_key_name) | |
| self._remove_gpg_key_if_exists(parameters.gpg_key_name) | |
| def _remove_gpg_key_if_exists(self, gpg_key_name: str) -> None: | |
| if gpg_key_name: | |
| remove_gpg_key_if_exists(gpg_key_name) |
| if parameters.gpg_key_name: | ||
| # Remove the GPG key (Optional) | ||
| remove_gpg_key_if_exists(parameters.gpg_key_name) |
There was a problem hiding this comment.
Suggestion: Consider checking if the file exists before attempting to remove it to provide a clearer error message. [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 parameters.gpg_key_name: | |
| # Remove the GPG key (Optional) | |
| remove_gpg_key_if_exists(parameters.gpg_key_name) | |
| if parameters.gpg_key_name: | |
| gpg_key_path = get_gpg_key_path(parameters.gpg_key_name) | |
| if not os.path.exists(gpg_key_path): | |
| logger.warning(f"GPG key file does not exist: {gpg_key_path}") | |
| remove_gpg_key_if_exists(parameters.gpg_key_name) |
| params = ApplicationAddSourceParameters( | ||
| file_name="google-chrome.sources", | ||
| sources="X-Repolib-Name: Google Chrome" | ||
| "Enabled: yes" | ||
| "Types: deb" | ||
| "URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub" | ||
| "Suites: stable" | ||
| "Components: main", |
There was a problem hiding this comment.
Suggestion: The string concatenation in the 'sources' field of 'ApplicationAddSourceParameters' seems to be missing spaces between the concatenated strings. This will result in an incorrect single string where the parts are not separated by spaces, which could lead to errors when parsing the sources. [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.
| params = ApplicationAddSourceParameters( | |
| file_name="google-chrome.sources", | |
| sources="X-Repolib-Name: Google Chrome" | |
| "Enabled: yes" | |
| "Types: deb" | |
| "URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub" | |
| "Suites: stable" | |
| "Components: main", | |
| params = ApplicationAddSourceParameters( | |
| file_name="google-chrome.sources", | |
| sources="X-Repolib-Name: Google Chrome " | |
| "Enabled: yes " | |
| "Types: deb " | |
| "URIs: https://dl-ssl.google.com/linux/linux_signing_key.pub " | |
| "Suites: stable " | |
| "Components: main", | |
| ) |
| def test_add_app_with_gpg_key_successfully(self): | ||
| try: | ||
| params = ApplicationAddSourceParameters( | ||
| file_name="intel-gpu-jammy.list", | ||
| sources="deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main", | ||
| gpg_key_uri="https://dl-ssl.google.com/linux/linux_signing_key.pub", | ||
| gpg_key_name="google-chrome.gpg" | ||
| ) | ||
| command = UbuntuApplicationSourceManager() | ||
| with patch("builtins.open", new_callable=mock_open()): | ||
| command.add(params) | ||
| except SourceError as err: | ||
| assert False, f"'UbuntuApplicationSourceManager.add' raised an exception {err}" |
There was a problem hiding this comment.
Suggestion: The 'test_add_app_with_gpg_key_successfully' and 'test_add_app_deb_822_format_successfully' methods should not suppress all exceptions with a broad try-except block. Instead, they should let the test framework handle exceptions, which will provide better error reporting and debugging information. [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.
| def test_add_app_with_gpg_key_successfully(self): | |
| try: | |
| params = ApplicationAddSourceParameters( | |
| file_name="intel-gpu-jammy.list", | |
| sources="deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main", | |
| gpg_key_uri="https://dl-ssl.google.com/linux/linux_signing_key.pub", | |
| gpg_key_name="google-chrome.gpg" | |
| ) | |
| command = UbuntuApplicationSourceManager() | |
| with patch("builtins.open", new_callable=mock_open()): | |
| command.add(params) | |
| except SourceError as err: | |
| assert False, f"'UbuntuApplicationSourceManager.add' raised an exception {err}" | |
| def test_add_app_with_gpg_key_successfully(self): | |
| params = ApplicationAddSourceParameters( | |
| file_name="intel-gpu-jammy.list", | |
| sources="deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main", | |
| gpg_key_uri="https://dl-ssl.google.com/linux/linux_signing_key.pub", | |
| gpg_key_name="google-chrome.gpg" | |
| ) | |
| command = UbuntuApplicationSourceManager() | |
| with patch("builtins.open", new_callable=mock_open()): | |
| command.add(params) |
| def test_update_app_source_successfully(self): | ||
| try: | ||
| params = ApplicationUpdateSourceParameters( | ||
| file_name="intel-gpu-jammy.list", sources=APP_SOURCE | ||
| ) | ||
| command = UbuntuApplicationSourceManager() | ||
| with patch("builtins.open", new_callable=mock_open()) as m: | ||
| with patch("builtins.open", new_callable=mock_open()): | ||
| command.update(params) | ||
| except SourceError as err: | ||
| assert False, f"'UbuntuApplicationSourceManager.update' raised an exception {err}" |
There was a problem hiding this comment.
Suggestion: The 'test_update_app_source_successfully' method should not suppress all exceptions with a broad try-except block. Instead, it should let the test framework handle exceptions, which will provide better error reporting and debugging information. [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.
| def test_update_app_source_successfully(self): | |
| try: | |
| params = ApplicationUpdateSourceParameters( | |
| file_name="intel-gpu-jammy.list", sources=APP_SOURCE | |
| ) | |
| command = UbuntuApplicationSourceManager() | |
| with patch("builtins.open", new_callable=mock_open()) as m: | |
| with patch("builtins.open", new_callable=mock_open()): | |
| command.update(params) | |
| except SourceError as err: | |
| assert False, f"'UbuntuApplicationSourceManager.update' raised an exception {err}" | |
| def test_update_app_source_successfully(self): | |
| params = ApplicationUpdateSourceParameters( | |
| file_name="intel-gpu-jammy.list", sources=APP_SOURCE | |
| ) | |
| command = UbuntuApplicationSourceManager() | |
| with patch("builtins.open", new_callable=mock_open()): | |
| command.update(params) |
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
Adds support for DEB822 format to the 'source application add' command.
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