-
Notifications
You must be signed in to change notification settings - Fork 592
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
chore(nms): Improve generic commands functionality #14014
chore(nms): Improve generic commands functionality #14014
Conversation
Thanks for opening a PR! 💯
Howto
More infoPlease take a moment to read through the Magma project's
If this is your first Magma PR, also consider reading
|
e5eed3c
to
499f929
Compare
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.
Looking good to me but hard to review with the linter noise. ;)
Some screenshots of the frontend help text and error message would be helpful.
c61d0ca
to
6aa86c4
Compare
I've addressed the linting noise and added some screenshots to the PR description. Let me know if there's anything else you'd like to see. |
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.
Looking good. If there's no easy way to handle the parameter error, I think it's fine to merge.
(You had a response interceptor in the code at some point. Is it not needed to show the correct error or did it get lost during rebase?)
6aa86c4
to
a3cf7dc
Compare
orc8r/gateway/python/magma/magmad/generic_command/command_executor.py
Outdated
Show resolved
Hide resolved
orc8r/gateway/python/magma/magmad/generic_command/command_executor.py
Outdated
Show resolved
Hide resolved
8e7eeb0
to
bc94e1f
Compare
orc8r/gateway/python/magma/magmad/generic_command/command_executor.py
Outdated
Show resolved
Hide resolved
Oops! Looks like you failed the Howto
♻️ Updated: ✅ The check is passing the Python Format Check after the last commit. |
bc94e1f
to
8eef16b
Compare
Work in progress, orc8r (and possibly NMS) needs to be adapted. Signed-off-by: Sebastian Thomas <sebastian.thomas@tngtech.com>
Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
3305a0b
to
e24d592
Compare
Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
e24d592
to
2e7e71c
Compare
* chore(magmad): Improve error handling of generic commands Work in progress, orc8r (and possibly NMS) needs to be adapted. Signed-off-by: Sebastian Thomas <sebastian.thomas@tngtech.com> * Propagate Not Found error to orc8r Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com> * Add information about configuration of generic commands Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com> * Change default value of parameters field to use shell_params Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com> * Handle further error cases Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com> * Allow generic command to run with parameters `{"shell_params": []}` Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com> Signed-off-by: Sebastian Thomas <sebastian.thomas@tngtech.com> Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com> Co-authored-by: Sebastian Thomas <sebastian.thomas@tngtech.com>
Summary
Builds on top of #13911. Closes #13894.
This PR:
This functionality could do with further improvements, so this is seen as a first step to make the current set-up more usable. An issue will be opened with further suggested improvements.
Test Plan
Command that is configured fails when no parameters specified
New default look
Command now works without modifying parameters field
A more helpful error is returned when a non-configured command is used