-
Notifications
You must be signed in to change notification settings - Fork 60
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
Contribute new pack for Cisco ISE #166
Conversation
6d0568a
to
d4e0ffe
Compare
Pending review and feedback. Otherwise pack is ready to merge. |
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 don't know ISE, so I can't evaluate any of the API usage.
However, I think using a common python entry point for all of these actions is a pretty sweet idea. I like how you've minimized things.
Do you have a script to generate all off the action metadata files from some kind of openapi spec? Or did you make all of them by hand?
I have several comments on python coding style. These are my gut reactions - I hope the feedback is helpful. Also, could you run black
on your python code? We've adopted black in core StackStorm, so that makes it a bit easier (more consistent) to review.
All by hand. Unfortunately the ISE API itself a bit inconsistent, and the documentation is even more so. It is rife with typos, incomplete data, missing structure, and some bit that are just plain wrong.
I am not familiar, but I will investigate. Thank you for the heads up!
Thank you! 😄
Feedback is always appreciated! I will review and reply to each, thank you for taking the time to do so. |
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.
One more minor nit, and then I think this is good to go. :)
Note: You'll still need to wait for someone on the TSC to migrate this over to its own repo.
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 this has incubated well enough. I can't actually test the pack, but it looks good to me from reading.
Anything else needed before this is approved? |
@namachieli A lot of the maintainers have been very busy with work and other things, so merging stuff can take a really long time. For example, given blag's work commitments he just announced that he's stepping down as a maintainer. And nmaludy switched jobs not too long ago, so he's been swamped with lots to do. Sorry about the delays getting this in! I can't officially approve this or create the git repo it needs to be in. Hopefully, someone will have time to do it sooner rather than later. Thank you for your patience, and, again, sorry this is taking awhile. |
Understood, thank you for replying. It's a tough situation for all. |
Just a note: the end goal of PRs here isn't actually to get merged. Somebody with enough access needs to either run this script: Or follow those steps to get this bootstrapped into StackStorm Exchange. Since I'm leaving, I think somebody else should take up the mantle here. I'm happy to lend a hand if they run into trouble, but I'd like to pass this knowledge on to somebody else to maintain that ability in the TSC. |
Transferred from: StackStorm-Exchange/exchange-incubator#166
Closing as https://github.com/stackStorm-exchange/stackstorm-cisco_ise pack is available in the StackStorm Index: https://exchange.stackstorm.org/#cisco_ise |
A Pack that uses requests and primarily yaml actions to interact with the Cisco ISE API.