-
Notifications
You must be signed in to change notification settings - Fork 34
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
Finish integrations #218
Conversation
- Added `/clear` command to Mattermost. - Improved integration workers loading and logging. - Sorted App config and environment variables. - Sorted pyproject configurations.
This PR is missing documentation updates yet, however opinions are welcome. ack @cmsirbu |
development/creds.example.env
Outdated
# SLACK_APP_TOKEN="changeme" | ||
# SLACK_SIGNING_SECRET="changeme" | ||
|
||
# - Webex ---------------------------- |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
.
- Added
- Removed
webex_teams
related deprecation warnings and usage. - Added
webex_msg_char_limit
App config entry.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposing to format /nautobot
and /clear
commands into the table, the same way as here: https://github.com/nautobot/nautobot-plugin-chatops/pull/218/files#diff-a382cc7dcacafa83ebf8f8dfa4e92f19e1eaa57265b5df03212fec6da0439f87R5
WDYT? @smk4664
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/dev/dev_environment.md
Outdated
TBD: Update after merging https://github.com/nautobot/nautobot-plugin-chatops/pull/212 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this before merge
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.
Fixed |
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. |
@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. |
Proposing to rename WDYT @smk4664 ? |
Now would be a good time to do this. I am good with aligning on these. |
I agree of the Cisco ACI docs. Thank you for pulling these in! |
docs/glossary.md
Outdated
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
docs/user/app_getting_started.md
Outdated
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`. | ||
|
||
{% |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Done |
There was a problem hiding this 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.
About
Finish adding integrations from baby plugins
What's Changed
/clear
command to development Mattermost.creds.example.env
.WebEx
=>Webex
.def get_app_config_part(prefix)
.webex_teams
related deprecation warnings and usage.webex_msg_char_limit
App config entry.TODO