-
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. |
closes #825