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

Slurpit Support #601

Closed
wants to merge 98 commits into from
Closed

Slurpit Support #601

wants to merge 98 commits into from

Conversation

lpconsulting321
Copy link
Contributor

Implements: #600

Added support for slurpit to sync locations, device types, devices, interfaces, inventory items, prefixes, vlans, vrf's and IP's.

Let me know if there are any issues.

@lpconsulting321 lpconsulting321 requested a review from a team as a code owner November 15, 2024 17:38
jdrew82
jdrew82 approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@jdrew82 jdrew82 left a comment

Choose a reason for hiding this comment

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

LGTM! Good work and awesome to see an async example with the contrib pattern!

@jdrew82 jdrew82 added the integration: slurpit Issues/PRs for Slurpit! integration. label Nov 15, 2024
@jdrew82
Copy link
Contributor

jdrew82 commented Nov 15, 2024

@lpconsulting321 One thing I just realized I forgot about. Can you update the CODEOWNERS in the github folder with who is responsible for the integration? Would it be just you or are there others?

@lpconsulting321
Copy link
Contributor Author

@jdrew82 Yeah. I wanted to use the built in Nautobot adapter as much as possible. Possible improvements for that in future would be to allow the ability allow get_queryset to pull based on tags/cf's I overcome this by just overiding the functions. Other than that it was perfect.

@lpconsulting321
Copy link
Contributor Author

@jdrew82

/nautobot_ssot/integrations/slurpit/ @lpconsulting321 @pietos @nautobot/plugin-ssot is this okay or shall i remove the last user?

@jdrew82
Copy link
Contributor

jdrew82 commented Nov 15, 2024

@jdrew82

/nautobot_ssot/integrations/slurpit/ @lpconsulting321 @pietos @nautobot/plugin-ssot is this okay or shall i remove the last user?

You can leave the plugin-ssot group as a fallback just in case it's something we can easily address.

@lpconsulting321
Copy link
Contributor Author

lpconsulting321 commented Nov 15, 2024 •
edited
Loading

Looks like pylint doesnt like some things

jdrew82
jdrew82 approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@jdrew82 jdrew82 left a comment

Choose a reason for hiding this comment

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

LGTM!

@lpconsulting321
Copy link
Contributor Author

lpconsulting321 commented Nov 15, 2024 •
edited
Loading

@jdrew82 Looks like unit tests are failing for non slurpit integrations. What do you do in these situations?

https://github.com/nautobot/nautobot-app-ssot/actions/runs/11861825954/job/33060798032?pr=601#step:9:5841

jdrew82
jdrew82 requested changes Nov 15, 2024
View reviewed changes
development/development.env Outdated
@@ -110,3 +110,6 @@ IPFABRIC_SSL_VERIFY="True"
IPFABRIC_TIMEOUT=15

NAUTOBOT_SSOT_ENABLE_ITENTIAL="True"

NAUTOBOT_SSOT_ENABLE_SLURPIT="True"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be False.

Copy link
Contributor

@jdrew82 jdrew82 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@lpconsulting321 That is most likely being caused due to your integration being enabled. Making this change should resolve the issue. Itential is the only integration that's left enabled for now as it has tests of the integration's Job that requires the integration be installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<3

@lpconsulting321
Copy link
Contributor Author

Ahh 3.8. The Slurpit SDK does not support 3.8 at the moment. We will make sure our SDK supports 3.8 and get this passing.

@jdrew82
Copy link
Contributor

jdrew82 commented Nov 20, 2024

@lpconsulting321 Nautobot 2.4 will be removing support for Python 3.8 so you might want to wait for that to be released and we can set that as minimum for this integration?

@jdrew82
Copy link
Contributor

jdrew82 commented Nov 20, 2024

Oh, also, before I forget, please make sure you update the docs in the README to include you integration in the list and the acknowledgements.

jdrew82 and others added 25 commits November 25, 2024 12:19
@jdrew82 @lpconsulting321
test: ✅ Add tests for CItrix ADM integration.
8c5470b
@jdrew82 @lpconsulting321
fix: 🐛 Fix missing ContentType bug in use of create_or_update_custom_…
dbbee53 
…field in signals.

This was caused by the apps not being loaded yet, so need to pass them to be found and used. Updated Bootstrap to fix the use of the function too.
@jdrew82 @lpconsulting321
docs: 📝 Add documentation for Citrix ADM integration.
4869887
@jdrew82 @lpconsulting321
build:
caaa2ab
@jdrew82 @lpconsulting321
test: ✅ Adjust tests for migration to SSoT project
fa136be
@jdrew82 @lpconsulting321
feat: ✨ Add additional information to Job like Citrix logo and Data m…
fb64b18 
…appings.
@jdrew82 @lpconsulting321
test: 🚨 Ignore pylint warning
0b51b9c
@jdrew82 @lpconsulting321
test: 🚨 Address complaints from pylint
8db8904
@jdrew82 @lpconsulting321
docs: 📝 Add changelog fragment bout parse_hostname_for_role() change.
1b95773
@lpconsulting321
conflict
b7cdb0c
@progala @lpconsulting321
Fix infite loop when resolving location parents.
cb113de
@progala @lpconsulting321
Replace region reference with parent.
7ab20d7
@progala @lpconsulting321
Disable unused job flag.
0703fb0
@progala @lpconsulting321
Implement SNOW SSOT 'delete_records' flag.
b599951
@progala @lpconsulting321
Add change fragments.
71be453
@progala @lpconsulting321
Fix linting error.
c05d3ac
@jdrew82 @lpconsulting321
docs: 📝 Add missing acknowledgements
8df8d9e
@jdrew82 @lpconsulting321
build: Add code owner for Citrix integration
7e2e394
@jdrew82 @lpconsulting321
docs: 📝 Add changelog snippets
bb3b229
@jdrew82 @lpconsulting321
fix: 🐛 Fix hostname_mapping option in Citrix ADM integration.
06a33cc
@jdrew82 @lpconsulting321
refactor: ♻️ Remove duplicate parse_hostname_for_role() in Meraki int…
f37f299 
…egration.

I missed this when I consolidated the functions in 599.
@jdrew82 @lpconsulting321
docs: 📝 Add changelog snippets
fd71fdc
@jdrew82 @lpconsulting321
fix: 🐛 Correct default to be a list.
8782719
@lpconsulting321
conflict
dde0c13
@lpconsulting321
update lock
6a9f874
@jdrew82 jdrew82 mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdrew82 jdrew82 jdrew82 requested changes

@glennmatthews glennmatthews Awaiting requested review from glennmatthews glennmatthews is a code owner

@qduk qduk Awaiting requested review from qduk qduk is a code owner

@bile0026 bile0026 Awaiting requested review from bile0026 bile0026 is a code owner

Assignees

@lpconsulting321 lpconsulting321

Labels
integration: slurpit Issues/PRs for Slurpit! integration. type: enhancement New feature or request type: major feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lpconsulting321 @jdrew82 @Renrut5 @progala