Skip to content
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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

rifen
Copy link

@rifen rifen commented Dec 17, 2024

  • Added configplan_confirmation view that shows the backup -> intended diffs for selected devices of each config plan selected in configplan_list. This then let's you press "Deploy Plan" that gives the modal, that deploys the job that pushes the config(as before).
  • I did testing and overall there is seemingly 1 issue:
    • The diff2html library used doesn't scale well with big diffs so those are limited to under 1k lines of differences but it works otherwise.
      • Most likely there is a more sleek way to deal with this but it hasn't came to mind yet.
        image

chrome_I1oM17wKK4

closes #825

@jeffkala
Copy link
Contributor

@rifen mind adding a few screenshots of what this change looks like from the UI?

@rifen
Copy link
Author

rifen commented Dec 18, 2024

@jeffkala Updated the description :) Was meaning to get to it.

@jeffkala
Copy link
Contributor

Does it work if the deploy job is scheduled? We had some issues with this in the past when doing things like this.

@rifen
Copy link
Author

rifen commented Dec 18, 2024

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.

@jeffkala
Copy link
Contributor

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.

@rifen
Copy link
Author

rifen commented Dec 19, 2024

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.

@jeffkala
Copy link
Contributor

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.

@itdependsnetworks
Copy link
Contributor

Yea, thinking about it further, I likely have similar concerns as @jeffkala. I did try to highlight to make this multiple PRs on #825 to try and make sure we did not have revery larger changes.

@jeffkala
Copy link
Contributor

jeffkala commented Dec 19, 2024

Going to just add some things as I'm going through my own testing

  1. Multiple configplans on a device and trying to deploy them at the same time gives unreadable dropdown titles.
    Screenshot 2024-12-19 at 1 18 28 PM
  2. When I get to the configplan confirmation pane, the only one I can actually open is the first one. The other 3 aren't clickable at all.

@jeffkala
Copy link
Contributor

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
Copy link
Contributor

  1. Create a Manual config plan for a device.
  2. set hostname rtr-01
  3. create config plan
  4. go to deploy config plan
  5. The diff because its actually only the intended vs backed up config doesn't accurately show the change you're making at all.

@jeffkala
Copy link
Contributor

Worried the htmldiff library might not be overly helpful here as well. Its not capable to intelligently diffing similar lines which makes a diff like the screenshot below hard to parse.
Screenshot 2024-12-19 at 1 59 12 PM
Screenshot 2024-12-19 at 1 59 21 PM

Took a bit out of the screenshots, but you can see ntp servers, lines etc. aren't even being diff'd in the correct side by side location.

@rifen
Copy link
Author

rifen commented Dec 19, 2024

Multiple configplans on a device and trying to deploy them at the same time gives unreadable dropdown titles.

@jeffkala Yes this needs to be addressed - though not entirely sure how you would combine them all this second but will work on it.

Yea, thinking about it further, I likely have similar concerns as @jeffkala. I did try to highlight to make this multiple PRs on #825 to try and make sure we did not have revery larger changes.

@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.

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 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.

  1. Create a Manual config plan for a device.
  2. set hostname rtr-01
  3. create config plan
  4. go to deploy config plan
  5. The diff because its actually only the intended vs backed up config doesn't accurately show the change you're making at all.

@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.

Worried the htmldiff library might not be overly helpful here as well. Its not capable to intelligently diffing similar lines which makes a diff like the screenshot below hard to parse.

@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.

@rifen
Copy link
Author

rifen commented Dec 20, 2024

Scaled config plan information better with icons and tooltips, looks more clean to me:
image

@rifen rifen requested a review from jeffkala December 30, 2024 15:22
@itdependsnetworks
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-Device Collapsing Config Diff
3 participants