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

Add plugins module. #203

Merged
merged 11 commits into from
May 11, 2023
Merged

Add plugins module. #203

merged 11 commits into from
May 11, 2023

Conversation

pszulczewski
Copy link
Contributor

@pszulczewski pszulczewski commented Apr 27, 2023

Proposal of dynamic plugins module.

This approach for plugins is slightly different than nautobot core modules, which have all endpoints statically added in the code. Looking at the variety of plugins/apps ecosystem I think that following the same patterns for plugins endpoints (a module for each endpoint) would be impractical, tedious to develop and hard to maintain in the future.

This proposal assumes that, plugin(plugin base_url), endpoint and object attributes are given in by users in ansible playbook task. Object attributes are split into two module args:

  1. id where object identifiers like name, slug etc. for endpoint.get(**id) are given as k:v pairs
  2. attrs where extra k:v pairs are given for creating or updating an object

Error handling is done by backend pynautobot and error message is returned in ansible task msg key.

Still TODO if these modules are welcomed:

  • Extend d-compose nautobot with a plugin.
  • Add integration test for the module.

@pszulczewski pszulczewski force-pushed the add_plugins branch 4 times, most recently from 036a2a7 to 05e304a Compare April 28, 2023 10:07
@jvanderaa
Copy link
Contributor

Loving what I'm seeing here. The only thing I'm not quite sure on yet is the id field. Would be good to come up with another name here. I was thinking data, but that is a previous module implementation detail. So would like to get something else.

@pszulczewski pszulczewski linked an issue May 9, 2023 that may be closed by this pull request
@pszulczewski pszulczewski marked this pull request as ready for review May 9, 2023 10:41
@pszulczewski
Copy link
Contributor Author

Loving what I'm seeing here. The only thing I'm not quite sure on yet is the id field. Would be good to come up with another name here. I was thinking data, but that is a previous module implementation detail. So would like to get something else.

This part identifies an object. We can have, name, slug as well other non-standard names like in case of bgp plugin asn - see integration tests.
I was thinking that id - identifier is short and clear about what it is. data is ambiguous IMO.

If you don't like id please give a specific name to replace and I will change it.
For me it can even be something_else :)

@joewesch
Copy link
Contributor

joewesch commented May 9, 2023

I also like the idea here, thanks. This seems like something that should definitely be included.

I also don't like just id, but I could be ok with an alias. Also, since it can take multiple identifiers can we make it plural?

  identifiers:
    aliases: [ids]

Lastly, can we possibly add a test to make sure we can account for nested plugin endpoints like we did for pynautobot? That may require adding the data-validation-engine plugin as well to the test environment as that is the only plugin I am aware of that uses them.

@pszulczewski
Copy link
Contributor Author

Thanks for the comment @joewesch. I like that. 👍

development/docker-compose.yml Outdated Show resolved Hide resolved
development/docker-compose.yml Outdated Show resolved Hide resolved
tests/integration/targets/latest/tasks/plugin_bgp_asn.yml Outdated Show resolved Hide resolved
plugins/modules/plugin.py Outdated Show resolved Hide resolved
plugins/modules/plugin.py Outdated Show resolved Hide resolved
@pszulczewski
Copy link
Contributor Author

Lastly, can we possibly add a test to make sure we can account for nested plugin endpoints like we did for pynautobot? That may require adding the data-validation-engine plugin as well to the test environment as that is the only plugin I am aware of that uses them.

I don't think we need that, as data-validation plugin has been standardized in the latest release 2.0.
see: nautobot/nautobot-app-data-validation-engine#29
Fixes API routes to be standardized (breaking change).
https://github.com/nautobot/nautobot-plugin-data-validation-engine/pull/29/files#diff-ad3b32d6e4e86b8fa1f80a5e6cad8f0eada338d217eb0f98c8d466de1eb6483d

@pszulczewski pszulczewski requested a review from joewesch May 10, 2023 09:50
@pszulczewski pszulczewski merged commit 6b0e19f into develop May 11, 2023
@pszulczewski pszulczewski deleted the add_plugins branch May 11, 2023 08:28
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.

New Module: Plugin Module
3 participants