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

Implement Custom Validators #157

Open
tyler-8 opened this issue Jan 15, 2023 · 15 comments
Open

Implement Custom Validators #157

tyler-8 opened this issue Jan 15, 2023 · 15 comments

Comments

@tyler-8
Copy link
Collaborator

tyler-8 commented Jan 15, 2023

I propose adding support for Custom Validators using the dotted path configuration method so users can still take advantage of the Dynamic Configuration support.

I'm already working on a branch to develop this but wanted to get an issue in place.

@lae
Copy link
Owner

lae commented Jan 18, 2023

Seems like this was introduced in 3.1.0? (netbox-community/netbox#7619) So from my perspective this is one of those features that should be configurable by this role in order to get 3.1.0 support through the door in the first place.

If I understand correctly this would fundamentally be the same as netbox_scripts/netbox_reports (with the slight difference of those validators being referenced in the custom validators config)?

@tyler-8
Copy link
Collaborator Author

tyler-8 commented Jan 18, 2023

They were part of the 3.0 release. They are slightly different from reports and scripts because there is no "root" folder that NetBox automatically looks for them in.

That means the user has to have a package/folder in the path so NetBox can followed the dotted path configured (I use local_validators in the same directory as manage.py).

Other than that, I am using similar logic to reports/scripts for copying & linking files.

@lae
Copy link
Owner

lae commented Jan 18, 2023

Oh I thought the custom validator support in 3.0 was just a specific DSL in configuration.py, not external files.

I just thought of something I've done before that might be useful. You can add directories to the PYTHONPATH at runtime:

import os
import sys
sys.path.insert(0, os.path.join(os.getcwd(), "{{ shared_directory }}/validators"))

That could go at the top of the configuration.py template. This way they don't necessarily have to be in the netbox folder itself, and might possibly be simpler to reason about (e.g. what if someone wanted to use a different package name than local_validators in their config?). Food for thought.

@tyler-8
Copy link
Collaborator Author

tyler-8 commented Jan 18, 2023

specific DSL in configuration.py, not external files.

It's both :)

The external files part is where you want to write a CustomValidator class that overrides the validate() method to do more complex validation than what the DSL provides. The dynamic configuration is "best" IMO and so there's no need to put anything in configuration.py (except perhaps your sys.path trick to get the path set up correctly) - and if users of this role want/need to define their validators in configuration.py, they still can using the role as it exists today, rather than adding new role-specific variables to handle it.

@sol1-matt
Copy link
Contributor

custom validators go in /srv/netbox/current/netbox/, I use name custom_validators.py but any name can be used.
the shared/configuration.py needs to look like this

CUSTOM_VALIDATORS = {
    "dcim.device": (
        'custom_validators.DeviceValidator',
    ),
    "virtualization.virtualmachine": (
        'custom_validators.VirtualMachineValidator',
    )
}

file deployment isn't hard but the template for configuration.py might need some work

if @lae is happy with the way I've done #190 I'll have a look into doing this in a similar way

@lae
Copy link
Owner

lae commented Nov 27, 2024

#190 is from my perspective a simpler feature because we're only copying a singular file there which NetBox seems to already be aware of.

Anyway feel free to have a go at this. From what I gather, I would add the snippet from earlier into the configuration.py.j2 template, create the shared directory it references, and copy the list of files specified in a role variable to that shared directory.

I'm not sure if the to_json filter works for the CUSTOM_VALIDATORS variable or not (cause they're tuples instead of lists?) but if it does, there shouldn't be any need to modify the template for it.

@sol1-matt
Copy link
Contributor

I think I tried, a fairly long time ago, to add the CUSTOM_VALIDATORS through this role and the output from to_json wasn't suitable because Netbox wanted the tuple. I'll have a look at it again.

My understanding of the custom validator file(s) is custom validators could use any file name or have multiple files.

But to make the automation simple I'd choose a static filename and everything can be put in there, that will make it similar to #190 in that we just need to deploy 1 file to shared, link it the right location and add the config to configuration.py.

@sol1-matt
Copy link
Contributor

sol1-matt commented Nov 29, 2024

I've begun to implement this here.
Need to run some more tests.

@tyler-8 if your still interested in this feature you should be able to test this.

@lae
Copy link
Owner

lae commented Dec 4, 2024

I'm kind of on the fence about it being a single file since it deviates from what Tyler and I discussed previously, so I'm postponing my review for it for a bit to give some time for Tyler (or anyone else who already use custom validators through external tasks) to pitch in their thoughts.

@tyler-8
Copy link
Collaborator Author

tyler-8 commented Dec 4, 2024

I think a single file is probably fine. I could see a scenario where someone has a LOT (15+) of custom validator classes and would want to organize them into separate files, but I suspect that would be more rare.

@sol1-matt
Copy link
Contributor

sol1-matt commented Dec 4, 2024

I've been thinking the way to use these is source: netbox_custom_validator.py -> dest: custom_validator.py so the source file, which is floating around in the files directory along with everything else a ansible install has, is netbox specific. And because it is a single file I can use a consistent name for the dest.

This is the same pattern I used with local_settings.py which I think will only ever be a single file.

I could change this to a list of files, update the readme to use a subdir so it is clear they are netbox files and not something else and then use {{ item | basename }} (I think that is the write ansible foo) to get the correct dest name.

netbox_custom_validators_files:
  - netbox/device_validator.py
  - netbox/vm_validator.py

edit: I'm only going to make this change if it is requested

@lae
Copy link
Owner

lae commented Dec 4, 2024

A single file is okay. I don't personally use custom validators myself so didn't really have any idea how involved real world setups may or may not be, so wanted to defer my judgement to someone who has.

@sol1-matt
Copy link
Contributor

I put all my validators in a single file because there are shared functions and this way I don't need to stuff around with getting the import right too. Also means I can be lazy with my ide setup (aka, none) :)

@sol1-matt
Copy link
Contributor

This patch was also tested with built in custom validators as well as the file based ones, the config deploy'd correctly for both.

CUSTOM_VALIDATORS = {
    "dcim.device": (
        'custom_validators.DeviceValidator',
    ),
    "virtualization.virtualmachine": (
        'custom_validators.VirtualMachineValidator',
    ),
   "dcim.site": [
        {
            "name": {
                "min_length": 5,
                "max_length": 30,
            }
        }
    ]
}

@lae
Copy link
Owner

lae commented Dec 4, 2024

I did notice your use of safe for that option and am mostly okay with it but it really does put the onus on the user to properly format it and ensure syntax is correct before deployment since it means the string is used as-is. 😅

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

No branches or pull requests

3 participants