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

Finish integrations #218

Merged
merged 26 commits into from
Jul 25, 2023
Merged

Finish integrations #218

merged 26 commits into from
Jul 25, 2023

Conversation

snaselj
Copy link
Contributor

@snaselj snaselj commented Jun 14, 2023

About

Finish adding integrations from baby plugins

What's Changed

  • Added /clear command to development Mattermost.
  • Improved integration workers loading.
  • Sorted App config and environment variables.
  • Sorted pyproject configurations.
  • Removed tests dependency on creds.example.env.
  • Added Cisco ACI documentation.
  • Renamed WebEx => Webex.
  • Simplified reading App config.
    • Added def get_app_config_part(prefix) .
  • Removed webex_teams related deprecation warnings and usage.
  • Added webex_msg_char_limit App config entry.
  • Added attributions for original contributors

TODO

  • Add Cisco Meraki documentation
  • Add Palo Alto Panorama documentation
  • Add AWX / Ansible Tower documentation
  • Add Grafana documentation
  • Add Arista CloudVision documentation
  • Add IP Fabric documentation
  • Update developer documentation

- Added `/clear` command to Mattermost.
- Improved integration workers loading and logging.
- Sorted App config and environment variables.
- Sorted pyproject configurations.
@snaselj snaselj marked this pull request as ready for review June 14, 2023 14:25
@snaselj
Copy link
Contributor Author

snaselj commented Jun 14, 2023

This PR is missing documentation updates yet, however opinions are welcome.

ack @cmsirbu

# SLACK_APP_TOKEN="changeme"
# SLACK_SIGNING_SECRET="changeme"

# - Webex ----------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

We went with Webex Teams when it was rebranded. Did Webex Teams become Webex again?

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, I know it doesn't really matter, as Admins can change the nautobot_config.py to whatever they want, it will still cause issues when using an older creds.env for developers expecting WEBEX_TEAMS_ACCESS_TOKEN.

Copy link
Contributor Author

@snaselj snaselj Jun 19, 2023

Choose a reason for hiding this comment

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

Expecting the next ChatOps App version to be 2.0, I was taking into account this deprecation: https://github.com/nautobot/nautobot-plugin-chatops/blob/develop/docs/admin/install/webex_setup.md#deprecation-warning

Webex Teams brand doesn't seem to be advertised by Ciso, when using search tools.

The old WEBEX_TEAMS_ACCESS_TOKEN is directly consumed by webexteamssdk library, however WEBEX_ACCESS_TOKEN is read by the App nautobot_config.py, passed to App config, and then passed to the library as arguments. Seems to me a bit better to use environment variables controlled by our App config to allow us some validations.

I would stay with Webex and emphasize this change in the documentation I'm preparing WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can replace all WebEx occurrences with Webex including class names in the code, to be inline with the official Cisco naming, WDYT @smk4664 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was going out pre 2.0, but we are close to the Nautobot 2.0 release, so I am good with these changes.
As long as we document them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Webex cleaned up:

  • Renamed WebEx => Webex.
  • Simplified reading App config.
    • Added def get_app_config_part(prefix) .
  • Removed webex_teams related deprecation warnings and usage.
  • Added webex_msg_char_limit App config entry.

@snaselj
Copy link
Contributor Author

snaselj commented Jun 20, 2023

Improved developer documentation and added Cisco ACI user and admin install documentation. Could you pls. check it? @smk4664

After agreed, I'll add the rest of the plugins documentation in the same style.

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

Choose a reason for hiding this comment

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

Sure! this would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 190 to 191
TBD: Update after merging https://github.com/nautobot/nautobot-plugin-chatops/pull/212

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this before merge

@snaselj snaselj requested a review from bryanculver June 20, 2023 12:20
@smk4664
Copy link
Contributor

smk4664 commented Jun 20, 2023

Could we also add attributions like @bryanculver did for the integrations we have already merged?

- Renamed `WebEx` => `Webex`.
- Simplified reading App config.
  - Added `def get_app_config_part(prefix)` .
- Removed `webex_teams` related deprecation warnings and usage.
- Added `webex_msg_char_limit` App config entry.
@snaselj
Copy link
Contributor Author

snaselj commented Jun 21, 2023

Could we also add attributions like @bryanculver did for the integrations we have already merged?

Fixed

@smk4664
Copy link
Contributor

smk4664 commented Jun 22, 2023

It looks to me like this PR should still be in Draft, with all the incomplete checkmarks. If this is not the case, I will finalize my review.

@snaselj snaselj marked this pull request as draft June 23, 2023 07:08
@snaselj
Copy link
Contributor Author

snaselj commented Jun 23, 2023

It looks to me like this PR should still be in Draft, with all the incomplete checkmarks. If this is not the case, I will finalize my review.

Converted to the Draft PR, will make it ready for review once all check marks will be done.

@snaselj
Copy link
Contributor Author

snaselj commented Jun 23, 2023

Improved developer documentation and added Cisco ACI user and admin install documentation. Could you pls. check it? @smk4664

After agreed, I'll add the rest of the plugins documentation in the same style.

@smk4664 What is your opinion on the ACI documentation in this PR? If you will find it sufficient, I'll add the rest of integrations docs in the same style.

@snaselj
Copy link
Contributor Author

snaselj commented Jul 10, 2023

Proposing to rename arista_cloudvision and related config keys to aristacv to be in sync with https://github.com/nautobot/nautobot-plugin-ssot/pull/138/files

WDYT @smk4664 ?

@smk4664
Copy link
Contributor

smk4664 commented Jul 10, 2023

Proposing to rename arista_cloudvision and related config keys to aristacv to be in sync with https://github.com/nautobot/nautobot-plugin-ssot/pull/138/files

WDYT @smk4664 ?

Now would be a good time to do this. I am good with aligning on these.

@smk4664
Copy link
Contributor

smk4664 commented Jul 10, 2023

Improved developer documentation and added Cisco ACI user and admin install documentation. Could you pls. check it? @smk4664

After agreed, I'll add the rest of the plugins documentation in the same style.

I agree of the Cisco ACI docs. Thank you for pulling these in!

docs/glossary.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this into the user guide, just below the "Description/Overview".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see it is included in the Development docs. I would also include it in the User Guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

pyproject.toml Outdated Show resolved Hide resolved
docs/dev/dev_environment.md Outdated Show resolved Hide resolved
docs/user/aci_commands.md Outdated Show resolved Hide resolved
All sub-commands are intended to be used with the `nautobot` prefix. For example, to retrieve a filtered list of VLANs, use the command `/nautobot get-vlans`.

{%
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need to see how it looks when all integrations have content, but I'm betting this will make for a lot of potential scrolling here. Maybe adding each page to the nav in an "Integrations commands" sub folder will be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

docs/admin/install/aci_setup.md Show resolved Hide resolved
@snaselj
Copy link
Contributor Author

snaselj commented Jul 19, 2023

Proposing to rename arista_cloudvision and related config keys to aristacv to be in sync with https://github.com/nautobot/nautobot-plugin-ssot/pull/138/files
WDYT @smk4664 ?

Now would be a good time to do this. I am good with aligning on these.

Done

@snaselj
Copy link
Contributor Author

snaselj commented Jul 19, 2023

Updated integrations commands reference and resolved most comments here.

@cmsirbu @smk4664

@snaselj snaselj marked this pull request as ready for review July 24, 2023 15:08
@snaselj
Copy link
Contributor Author

snaselj commented Jul 24, 2023

This PR is ready for review now.

cc: @cmsirbu @smk4664

Copy link
Contributor

@smk4664 smk4664 left a comment

Choose a reason for hiding this comment

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

Thanks for all the hard work! I will merge in and create a new release for this.

@smk4664 smk4664 merged commit 0a93a77 into develop Jul 25, 2023
34 checks passed
@smk4664 smk4664 deleted the u/snaselj-finish-integrations branch July 25, 2023 13:13
This was referenced Jul 25, 2023
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

3 participants