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

External Integrations #398

Merged
merged 10 commits into from Apr 16, 2024
Merged

Conversation

snaselj
Copy link
Contributor

@snaselj snaselj commented Mar 19, 2024

Closes: NaN

What's Changed

  • Added external integration to example jobs.
  • Added external integration to Arista Cloud Vision jobs.

@snaselj snaselj self-assigned this Mar 19, 2024
@snaselj
Copy link
Contributor Author

snaselj commented Mar 21, 2024

Added ExternalIntegration to AristaCV jobs. Could you pls. check it? @cmsirbu @jdrew82

@jdrew82
Copy link
Contributor

jdrew82 commented Mar 21, 2024

Added ExternalIntegration to AristaCV jobs. Could you pls. check it? @cmsirbu @jdrew82

I looked at what you'd done and good idea with the config information but I'm not sure how that'd work for the configuration information though as that's technically static in most cases and without the ExternalIntegration being defined at time of Job run I'd be curious how it'd show data. Have you tested that bit?

@snaselj
Copy link
Contributor Author

snaselj commented Mar 21, 2024

Added ExternalIntegration to AristaCV jobs. Could you pls. check it? @cmsirbu @jdrew82

I looked at what you'd done and good idea with the config information but I'm not sure how that'd work for the configuration information though as that's technically static in most cases and without the ExternalIntegration being defined at time of Job run I'd be curious how it'd show data. Have you tested that bit?

Without ExternalIntegration defined, this should work as before. If it's defined and specified as a job input argument, it takes precedence over PLUGIN_CONFIG values. Hope this is what was requested @cmsirbu

Have tested without real job run, just to create an integration, and call the job with it. It reads the data as expected. Would be great if someone with access to Arista CVP could test this.

@cmsirbu
Copy link
Contributor

cmsirbu commented Mar 21, 2024

The general idea was to avoid making this a breaking change - which means that if someone upgrades to the next release of SSoT and uses the PLUGIN_CONFIG method, their setup will continue working without any change.

@jdrew82
Copy link
Contributor

jdrew82 commented Mar 22, 2024

Added ExternalIntegration to AristaCV jobs. Could you pls. check it? @cmsirbu @jdrew82

I looked at what you'd done and good idea with the config information but I'm not sure how that'd work for the configuration information though as that's technically static in most cases and without the ExternalIntegration being defined at time of Job run I'd be curious how it'd show data. Have you tested that bit?

Without ExternalIntegration defined, this should work as before. If it's defined and specified as a job input argument, it takes precedence over PLUGIN_CONFIG values. Hope this is what was requested @cmsirbu

Have tested without real job run, just to create an integration, and call the job with it. It reads the data as expected. Would be great if someone with access to Arista CVP could test this.

I'm talking about the Config Information that shows on the DataSource/DataTarget pages. I'm pretty sure that's read at App load and isn't able to be dynamically updated by the ExternalIntegration selection.

@snaselj
Copy link
Contributor Author

snaselj commented Mar 25, 2024

Added ExternalIntegration to AristaCV jobs. Could you pls. check it? @cmsirbu @jdrew82

I looked at what you'd done and good idea with the config information but I'm not sure how that'd work for the configuration information though as that's technically static in most cases and without the ExternalIntegration being defined at time of Job run I'd be curious how it'd show data. Have you tested that bit?

Without ExternalIntegration defined, this should work as before. If it's defined and specified as a job input argument, it takes precedence over PLUGIN_CONFIG values. Hope this is what was requested @cmsirbu
Have tested without real job run, just to create an integration, and call the job with it. It reads the data as expected. Would be great if someone with access to Arista CVP could test this.

I'm talking about the Config Information that shows on the DataSource/DataTarget pages. I'm pretty sure that's read at App load and isn't able to be dynamically updated by the ExternalIntegration selection.

I've refactored, how the configuration is gathered for CloudVision. There is a new optional key in PLUGIN_CONFIG["nautobot_ssot"]["aristacv_external_integration_name"] that can be used to specify integration to use.

@snaselj snaselj marked this pull request as ready for review March 25, 2024 15:21
@snaselj snaselj requested review from qduk, jdrew82 and a team as code owners March 25, 2024 15:22
@Kircheneer
Copy link
Contributor

Can you provide an invoke command for the script you mentioned in your PR description? I think this would aid development in the future

@snaselj
Copy link
Contributor Author

snaselj commented Apr 3, 2024

Can you provide an invoke command for the script you mentioned in your PR description? I think this would aid development in the future

Would rather do this in a separate PR not to block this one.

@snaselj snaselj mentioned this pull request Apr 10, 2024
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.

Few things to address but this looks good in general.

nautobot_ssot/jobs/examples.py Show resolved Hide resolved
nautobot_ssot/integrations/aristacv/utils/nautobot.py Outdated Show resolved Hide resolved
Co-authored-by: Justin Drew <2396364+jdrew82@users.noreply.github.com>
@snaselj snaselj requested a review from jdrew82 April 11, 2024 06:34
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!

@jdrew82 jdrew82 merged commit 58716bc into develop Apr 16, 2024
16 checks passed
@jdrew82 jdrew82 deleted the u/snaselj-napps-276-external-integrations branch April 16, 2024 14:56
This was referenced Apr 16, 2024
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.

None yet

4 participants