-
Notifications
You must be signed in to change notification settings - Fork 58
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
Multi-Device Collapsing Config Diff #851
base: develop
Are you sure you want to change the base?
Conversation
@rifen mind adding a few screenshots of what this change looks like from the UI? |
@jeffkala Updated the description :) Was meaning to get to it. |
Does it work if the deploy job is scheduled? We had some issues with this in the past when doing things like this. |
@jeffkala I'm confused - wouldn't this be outside the scheduling of a config plan to be deployed as a job? There wouldn't really be confirmation if it's already happening automatically(scheduled). 🤔 Is there a different way of scheduling config plans, maybe I am not aware of. |
Sorry, I meant Job approvals, not scheduled jobs. It'd expect its going to work fine relooking at the code. |
@jeffkala Yes it works. If the config plan isn't approved, the underlying job still fails. A next feature might be implementing an approve all button or something of that sort in the config plan confirmation screen. So that you don't have to go back and select all the config plans again - change the statuses and select them again to deploy. |
Going to pull down this PR and play with it today. I have some internalized concerns with it scaling but need to pull down to vet those out. |
Going to just add some things as I'm going through my own testing |
nautobot_golden_config/templates/nautobot_golden_config/configplan_confirmation.html
Show resolved
Hide resolved
General feeling with this view is I could see it much more useful as a "bulk plan approval" view. Meaning I created a ton of configplans. I want to see them all with their diffs and have a checkbox for each as I go through them to say "approve" etc. Very similar to github PR review where you have all the file changes and a button that says "viewed file". |
|
@jeffkala Yes this needs to be addressed - though not entirely sure how you would combine them all this second but will work on it.
@itdependsnetworks I'm open to suggestions, this doesn't seem like a large change to me. I am not sure how to make it smaller overall.
@jeffkala Totally! Could add a checkbox even to the collapsible or something along those lines. Loading the diffs in all at once without the collapsible, will be a big browser performance hit.
@jeffkala Forsure - open to suggestions, I could show the config set but there really wouldn't be a diff because there could be no backup/intended/compliance in those scenarios, if we going to the device (EOS for example) we could do config sessions and get a diff that way but that's not what golden config does.
@jeffkala I'm worried about it too in all honesty - I looked for alternatives but didn't really come up on anything and it's what is used elsewhere in GC. |
@jeffkala is going to re-test, but in our convo today, but to set expectations I don't think html diff is going to be a good fit here. It is likely to cause more confusion most of the time. |
@rifen planning on retesting today. May need to remove htmldiff to get it accepted but a majority of this looks good so far. |
@jeffkala Sweet - look forward to the results. |
@jeffkala Internally we have parallel efforts going on, we are using https://github.com/otakustay/react-diff-view I don't know personally how this could be used in Nautobot though but maybe it could. Seems to scale better in our testing with higher accuracy. |
There is a PR opened into core for monaco-diff-viewer too. Which seems like a good fit when it gets in. so far all the other changes look great. |
@rifen, would you see this helpful to have the collapsed config plan confirmation page without the diff? Maybe just providing all changes to be made? |
How does it work for manual config pushes? |
closes #825