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

Remove conditional views in Golden Config #847

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

Conversation

jtdub
Copy link

@jtdub jtdub commented Dec 11, 2024

Closes #842.

  1. Removes conditional based UI views
  2. Allows configurations to be updated in the Golden Config Settings UI.

Navigation views display all options regardless of what is enabled or disabled:

Screenshot 2024-12-19 at 6 28 06 PM

Golden Config Settings displays whether backups and intended are enabled:

Screenshot 2024-12-19 at 6 29 54 PM

Creating and Updating Golden Config Settings in the UI:

Screenshot 2024-12-19 at 3 28 39 PM Screenshot 2024-12-19 at 3 28 50 PM

Reading the Golden Config Settings in the UI.

Screenshot 2024-12-19 at 3 30 02 PM

@jtdub jtdub self-assigned this Dec 11, 2024
@jtdub jtdub marked this pull request as draft December 11, 2024 18:44
@jtdub jtdub force-pushed the gc-app-settings branch 2 times, most recently from 47e4b89 to 3113bf9 Compare December 13, 2024 15:32
@jtdub jtdub marked this pull request as ready for review December 16, 2024 19:00
@jtdub jtdub changed the title WIP | Remove conditional views in Golden Config Remove conditional views in Golden Config Dec 16, 2024
@jeffkala
Copy link
Contributor

jeffkala commented Dec 18, 2024

  • content_template.html - Does have some conditional views logic as well. But perhaps its one that makes some since since its on the device detail side. Just notating here for review purposes for now, no action right now.
  • goldenconfig_list.html - Has some conditional view logic that needs to figure out if we should remove as well.

@jeffkala
Copy link
Contributor

AS far as this concern: "UI-based configuration settings would apply to each device individually within a dynamic group, rather than checking the configuration globally once."

I don't expect this to cause much performance issue as we already have function get_device_to_settings_map that does most of this work. I think its still one call to that function to create the map and then its a call to the map.

@jtdub jtdub marked this pull request as draft December 19, 2024 21:30
@jeffkala
Copy link
Contributor

jeffkala commented Dec 19, 2024

List view for GC settings has two different types of checkbox.
Screenshot 2024-12-19 at 3 02 11 PM

Also not sure we really need all these in the table view.

tasks.py Outdated Show resolved Hide resolved
@jtdub jtdub marked this pull request as ready for review December 27, 2024 22:02
@jtdub jtdub requested a review from jeffkala December 27, 2024 22:02
@itdependsnetworks itdependsnetworks changed the base branch from develop to main January 9, 2025 20:28
@itdependsnetworks itdependsnetworks changed the base branch from main to develop January 9, 2025 20:28
@jeffkala
Copy link
Contributor

Doing a full review of this today.

@jeffkala
Copy link
Contributor

@jtdub here's my first review. Many seems like we need to harden the checks before things execute based on GC setting (setting) variable.

  1. GC setting for only backing up devices with everything else turned off.
    1. The assumption would be you don't need SOT_AGG query. Gives an error.
    2. Running the intended job on the device where intended is disabled gives error E3018: Missing the required global setting: jinja_path_template.
      1. Would be good to give a better error (intended generation is not enabled or similar)
    3. Similar with all the jobs, odd errors are given when jobs are run against device that doesn't have that "setting" enabled.
  2. If a device previously had been in-scope for say "config compliance" and you change it to out of scope the device still shows in the config compliance list view
    1. May need some kind of cleanup? (TBD)
    2. Or just document as part of an migration guide
  3. When config-plans are disabled. You can still create a config plan for a out of scope device. (Might be due to previous bullet point)
    1. You can also still deploy a config plan when deployment is disabled.
  4. Generally jobs run that include elements of settings that are currently turned off the error Missing the required global setting: may need to be more verbose Missing the required global setting as the golden config setting is disabled or something.
    1. Seems like proper log entries for "feature x is disabled" works for the backup job when backups are disabled for a device.

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.

Make Global App config options available on a per golden config setting level
2 participants