Skip to content

Conversation

@irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingoujAtDevolution commented Feb 12, 2024

Add mdns to network scan, added some extra helper method for better readbility and maintainbility.

Open for review but needs to wait for the #689 to be merged first

Copy link
Contributor

@pauldumais pauldumais left a comment

Choose a reason for hiding this comment

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

Wow this is awesome, LGTM

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Nice! I’m blocking the merge because the retry loop logic seems erroneous to me. Can you double check?

Comment on lines +2011 to +2036
"polling 2.8.0",
"socket2 0.4.10",
Copy link
Member

@CBenoit CBenoit Feb 13, 2024

Choose a reason for hiding this comment

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

issue (non-blocking): This crate is using outdated dependencies, and this causes dependency duplication. Do you think you could send a PR for updating them soonish? It’s okay to wait until you are done with implementing the feature first. If you don’t do it now, can you create a Jira ticket to not forget? Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created PR to mdsn-sd

Comment on lines -302 to +352
pub ping_interval: u64, // in milliseconds
pub ping_timeout: u64, // in milliseconds
pub broadcast_timeout: u64, // in milliseconds
pub port_scan_timeout: u64, // in milliseconds
pub netbios_timeout: u64, // in milliseconds
pub netbios_interval: u64, // in milliseconds
pub max_wait_time: u64, // max_wait for entire scan duration in milliseconds, suggested!
pub ping_interval: u64, // in milliseconds
pub ping_timeout: u64, // in milliseconds
pub broadcast_timeout: u64, // in milliseconds
pub port_scan_timeout: u64, // in milliseconds
pub netbios_timeout: u64, // in milliseconds
pub netbios_interval: u64, // in milliseconds
pub mdns_meta_query_timeout: u64, // in milliseconds
pub mdns_single_query_timeout: u64, // in milliseconds
pub max_wait_time: u64, // max_wait for entire scan duration in milliseconds, suggested!
Copy link
Member

Choose a reason for hiding this comment

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

thought: We should probably use the Duration type instead. Should be addressed separately.

@CBenoit CBenoit marked this pull request as draft February 13, 2024 08:59
@CBenoit
Copy link
Member

CBenoit commented Feb 13, 2024

Open for review but needs to wait for the #689 to be merged first

Converted to draft to make it obvious.

@CBenoit CBenoit changed the title feat(dgw):add mdns for network scan feat(dgw): mDNS network scan support Feb 13, 2024
@irvingoujAtDevolution irvingoujAtDevolution marked this pull request as ready for review February 13, 2024 18:25
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Thank you! Looks mostly good. I spotted two other issues which should be fixed, and then we can merge.

.nest("/jet/config", config::make_router(state.clone()))
.nest("/jet/session", session::make_router(state.clone()))
.nest("/jet/sessions", sessions::make_router(state.clone()))
.nest("/jet/sessions", session::make_router(state.clone()))
Copy link
Member

Choose a reason for hiding this comment

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

issue: Any reason for this? This looks very problematic (creating the wrong router).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a mistake, thank you for point it out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@CBenoit CBenoit enabled auto-merge (squash) February 14, 2024 03:55
@irvingoujAtDevolution
Copy link
Contributor Author

LGTM! 🚀

Merci!

@CBenoit CBenoit merged commit 043a2b7 into master Feb 14, 2024
@CBenoit CBenoit deleted the Network-Scan-Zero-Conf branch February 14, 2024 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants