Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[uniconfig] add device discovery task #23

Merged
merged 4 commits into from
Dec 14, 2023
Merged

[uniconfig] add device discovery task #23

merged 4 commits into from
Dec 14, 2023

Conversation

MarcelSuleiman
Copy link
Contributor

No description provided.

@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch 4 times, most recently from 1b5f4b9 to 65a385c Compare November 23, 2023 14:03
@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch 6 times, most recently from 66f6c58 to e0e10c4 Compare November 29, 2023 15:31
@Jozefiel Jozefiel requested a review from marosmars November 29, 2023 16:00
@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch from e0e10c4 to 7094d2d Compare November 29, 2023 16:13
@Jozefiel
Copy link
Contributor

1: move tests to python folder. should not be a part of package
2: remove classmethod from validators, mypy should be configured for this use case
3: try to fix mypy issues instead of ignoring

@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch from 7094d2d to bb6aab3 Compare November 29, 2023 16:16
@MarcelSuleiman
Copy link
Contributor Author

2: remove classmethod from validators, mypy should be configured for this use case
when I remove @classmethod I will recieve pydantic.errors.PydanticUserError: @field_validator cannot be applied to instance methods ... I inspired by pydantic docs how to solve this situation

@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch from bb6aab3 to be02ee7 Compare November 29, 2023 16:30
@Jozefiel
Copy link
Contributor

2: remove classmethod from validators, mypy should be configured for this use case
when I remove @classmethod I will recieve pydantic.errors.PydanticUserError: @field_validator cannot be applied to instance methods ... I inspired by pydantic docs how to solve this situation

Check this PR. Mypy pass with removed classmethod
#29

@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch 3 times, most recently from 8b43df9 to 6c2afe1 Compare November 30, 2023 11:50
@MarcelSuleiman MarcelSuleiman force-pushed the suleiman/device_discovery branch from 44bc86e to 1a8f91a Compare November 30, 2023 14:06
jaro0149
jaro0149 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Collaborator

@jaro0149 jaro0149 left a comment

Choose a reason for hiding this comment

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

just few small comments

uniconfig/python/frinx_worker/uniconfig/device_discovery.py
class WorkerInput(TaskInput):
ip: list[Address]
tcp_port: list[TcpPortItem] | None = None
udp_port: list[UdpPortItem] | None = Field(None, max_length=500)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason why udp list has max_length but tcp list doesn't have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is because UDP port scanning is very slow. If I didn't have it restricted, the task crashed after 120 seconds on timeout error. Maros gave me a tip that I can limit the maximum number of ports - number 500 I set "randomly" as the maximum value when the task is not yet crashing...

uniconfig/python/frinx_worker/uniconfig/util.py
def get_list_of_ip_addresses(ip_addresses: str) -> list[str]:
"""
Creates list of IP addresses.
Args:
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is better if you use standard python tags in doc

@marosmars marosmars merged commit 62b7ad1 into main Dec 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaro0149 jaro0149 jaro0149 approved these changes

@marosmars marosmars marosmars approved these changes

@Jozefiel Jozefiel Awaiting requested review from Jozefiel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcelSuleiman @Jozefiel @marosmars @jaro0149