Skip to content

Conversation

@AndryNick98
Copy link
Collaborator

Related Issue

#865

Description

Fix the problem that occur when inside an inventory file be set https as transport but the port remain 22 instead of 443.
Modified set_device method inside source class to set the correct default port based on transport.
Fix mismanagement about ignore_known_hosts property.
Updated test.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Double Check

  • I have read the comments and followed the CONTRIBUTING.md.
  • I have explained my PR according to the information in the comments or in a linked issue.
  • My PR source branch is created from the develop branch.
  • My PR targets the develop branch.
  • All my commits have --signoff applied

Copy link
Collaborator

@LucaNicosia LucaNicosia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a linting error to fix

Comment on lines -83 to +168
for key, node in inventory.items():
for key, node in result_inventory.items():
Copy link
Collaborator

@LucaNicosia LucaNicosia May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your fix is absolutely correct, but now you don't need to compare one element at a time anymore. You can simply compare inv with result_inventory

Suggested change
for key, node in inventory.items():
for key, node in result_inventory.items():
assert inv == result_inventory, 'inventories do not match'

@AndryNick98 AndryNick98 force-pushed the fix-inventory-set-device branch from 7d63fc6 to 06f52df Compare May 31, 2023 13:08
Signed-off-by: AndryNick98 <andrea.nicoletti@stardustsystems.net>
Signed-off-by: AndryNick98 <andrea.nicoletti@stardustsystems.net>
@AndryNick98 AndryNick98 force-pushed the fix-inventory-set-device branch from 215a1ee to ded9fa1 Compare May 31, 2023 13:25
@ddutt ddutt merged commit 0c0d39e into netenglabs:develop Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants