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.

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

feat: Refactor mech driver #500

Merged
merged 8 commits into from
Nov 25, 2024
Merged

feat: Refactor mech driver #500

merged 8 commits into from
Nov 25, 2024

Conversation

mfencik
Copy link
Contributor

@mfencik mfencik commented Nov 19, 2024

After this is tested and merged, I'm aware we can do a better cleanup in workflows as we won't need undersync_device workflow anymore.
This PR needs another change in Nautobot prep_switch_interface job as from now on we can use connected_interface_uuid instead of mac_address, so please leave merging to me.

Copy link
Contributor

@stevekeay stevekeay left a comment

Choose a reason for hiding this comment

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

Timeouts were recently added to prevent the undersync workflow from hanging indefinitely. It might be a good idea to do that here, too.

@mfencik mfencik force-pushed the refactor_mech_driver branch 2 times, most recently from 237e5ae to af84243 Compare November 25, 2024 15:09
@mfencik
Copy link
Contributor Author

mfencik commented Nov 25, 2024

Timeouts were recently added to prevent the undersync workflow from hanging indefinitely. It might be a good idea to do that here, too.

This is implemented too

@mfencik mfencik marked this pull request as ready for review November 25, 2024 18:46
Copy link
Collaborator

@skrobul skrobul left a comment

Choose a reason for hiding this comment

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

overall LGTM. Added few more suggestions for missing docstrings and type hints.

One more (optional) idea - consider updating #update_nautobot to return an instance of UUID instead of plain string, this way we add another layer of data validation which would be better than a docstring promise of returning string that happens to contain uuid.

@mfencik
Copy link
Contributor Author

mfencik commented Nov 25, 2024

overall LGTM. Added few more suggestions for missing docstrings and type hints.

One more (optional) idea - consider updating #update_nautobot to return an instance of UUID instead of plain string, this way we add another layer of data validation which would be better than a docstring promise of returning string that happens to contain uuid.

that's a good idea, implemented

@mfencik mfencik added this pull request to the merge queue Nov 25, 2024
Merged via the queue into main with commit b72d491 Nov 25, 2024
25 checks passed
@mfencik mfencik deleted the refactor_mech_driver branch November 25, 2024 22:51
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.

3 participants