Support for installing/configuring CUSTOM_VALIDATORS

[sol1-matt]

#157 Adds support custom validation rules and custom validation logic.

[lae]

See comments.

Also, it's preferred that you rebase against develop when syncing or amending commits for minor fixes, and then force pushing the result afterwards. This is mainly to keep commit history succinct—otherwise I'll squash them on merge but that'll result in losing your direct authorship (if that matters to you).

The wrapping on the doc updates is also a bit off—it doesn't seem to be following either Asciidoc convention or a character limit. The convention is to break lines at the end of a sentence (i.e. one sentence per line), but I've also wrapped on commas or end of a clause if a line is particularly long.

[lae]

Compared to local_settings.py, as far as I can tell custom_validators.py is not an expected file that gets imported in the upstream NetBox codebase. Having not tested this I'm assuming dropping it in the netbox folder makes it reachable from NetBox as custom_validators.XYZ and is why it's being symlinked here?

This isn't really an issue for stable NetBox deployments, but if I recall correctly it prevents updates for git-based deployments because the NetBox directory ends up having uncommitted changes. Which means we have to use a different method of making this file importable, maybe via installing the file into the virtualenv's site-packages directory directly?

[lae]

Also extra note, in #191 I moved all configuration file changes into a block so the handler is only specified once. Something to look out for when rebasing.

[lae]

Okay, this looks good overall.

The "action" pattern being used here in the role variables is kind of novel to me. Do you know of any other roles that handle files or something similar this way? From a practical perspective it seems fine, but from a UX perspective maybe a bit jarring? If any of the other contributors/other users passing by see this, what do you think? (probably better to ask now than have to think about backwards compatibility later if I do end up changing how the role variables work)

[lae]

I guess I'll add Tyler (if you have the time) since he opened the original ticket too.

[sol1-matt]

The "action" pattern being used here in the role variables is kind of novel to me.

The reason for the action pattern is because we don't have a fixed file.

With similar "copy file to share and add/remove symlink" tasks currently in the role they work because the destination file is hard coded so we know the name of the file to remove if required.

With the way I built this you can choose the name or have multiple files with different names for both validation so the name of a file to be remove can only be expressed with a variable.

The though process behind the naming is

• I want to use this now
• I want to deploy this so I can turn it on and off in config myself (testing) OR I want to stop using this but leave the file in shared
• I want to remove something that existed previously

[sol1-matt]

Do you know of any other roles that handle files or something similar this way?

I kinda based it of ansible itself like ansible.builtin.file but I didn't think state was a good fit so a used action instead.

Welcome anybody else's thoughts on this.

[sol1-matt]

@lae another change I made to get custom validators working was to change the configuration template so it was easier to setup the yml and output'd in a easier to read format.

I've got that change on my branch readable_config

I had 2 problems with the way it currently worked created config

• at times I couldn't output a valid config (could have been a user problem)
• when I had problems, trying to "see" problems in the config was harder

I'm not sure what I've done here is "better", I think it is likely less safe, but it helped me with the 2 above problems.

If this is something you'd like to consider I'm happy to tidy up that branch and submit another PR. But I acknowledge that this may not be the direction you'd like the role to go in which is why I kept it separate from deploying the files.

[lae]

With the way I built this...

Yeah, I totally get how it works and is intended to work, and it very much reminded me of the state attribute in file-related modules like you mention. It makes sense and the documentation explains it well enough. I've just never seen role variables used in this fashion, and now that I've had some time to mull over it I think what's jarring is that it may be overengineering/overloading the role variables a bit.

• leaving files in shared in a different "state" than I guess what you could say "installed" seems questionable. the source playbook should have them in version control already, and netbox doesn't really access the shared directory directly (afaik?) so I guess I don't really understand what it is exactly that leaving it copied but not symlinked enables for an end user.
• removing files could (should?) be as simple as just removing it from the list. this would be doable with a fully-managed validators directory

...and now I noticed that I overlooked something in the earlier review. This comment from last year I think probably still applies here with the symlinks? But thinking about that comment I might be wrong—they shouldn't be tracked files so I normally git pull etc should ignore those files and work fine but maybe ansible.builtin.git is picky about having a clean directory i.e. any untracked files are in the .gitignore?

[sol1-matt]

leaving files in shared in a different "state" than I guess what you could say "installed" seems questionable. the source playbook should have them in version control already, and netbox doesn't really access the shared directory directly (afaik?) so I guess I don't really understand what it is exactly that leaving it copied but not symlinked enables for an end user.

This could be truma from setting up SSO :). The workflow I've done in the past is base install with ansible, have multiple possible files deploy'd to shared with ansible. Turn them on and off in config as I work on SSO without running ansible each time.

Happy to remove the option.

removing files could (should?) be as simple as just removing it from the list. this would be doable with a fully-managed validators directory

The file names aren't fixed, users can named them whatever they like and have multiple files. The reference in Netbox is in the config to the filename.

eg:
filenames

netbox_custom_validators_file:
  - validator_dcim.py
  - validator_virtualization.py

are referenced in config as

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

Same sort of thing with the python social auth pipelines.

But thinking about that comment I might be wrong—they shouldn't be tracked files so I normally git pull etc should ignore those files and work fine but maybe ansible.builtin.git is picky about having a clean directory i.e. any untracked files are in the .gitignore?

I'll look into if ansible.builtin.git will ignore the symlinks or a way to make ansible.builtin.git ignore the symlinks.

[lae]

The file names aren't fixed, users can named them whatever they like and have multiple files.

Right. By fully-managed validators directory I mean that Ansible would remove any unexpected files in said directory, if they happened to be removed from (or never existed in) the list. I think you could achieve this by globbing the files in the destination directory, then applying state: absent for any items not in the role variable.

[sol1-matt]

By fully-managed validators directory I mean that Ansible would remove any unexpected files in said directory

I think what you are saying is I need to create a destination directory for validator and pipline files current current/netbox/custom_validators, current/netbox/netbox/custom_pipelines and shared shared/custom_validators, shared/custom_pipelines

From this Ansible can

• add/remove/update files in the shared directory
• add/remove symlinks from the shared directory to the current directory

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

The Netbox config would be something like this which is non standard and I'd need to test if it works

This way seems to be a lot of complexity for the benefit of ansible configuration simplicity though I can see the benefit of the role making Netbox "clean" if humans do a bunch of manual stuff (also a bit of danger there too :).

An alternate to not having a current directory would be to just use files and then detect and remove orphaned symlinks.
No way for the role to remove manually added files then but I it is reasonable for the role to expect them to be manually removed as well.

I think having a shared directory and removing orphaned symlinks is the better alternative if you don't want explicit deploy, copy, remove parameters in ansible config.

[lae]

If the symlink method works for git-based installs (i.e. how this PR currently works) then the custom_validators and custom_pipelines directory names can still be made irrelevant. You'd just symlink/delink the files themselves (not the custom_validators directory) into current in conjunction with the copy/file tasks.

If the symlink method does cause update issues for git-based deploys, then I think, per this comment, that you can load those directories directly from e.g. shared/custom_validators and it should still let you reference your own validators without the parent directory name. But this is definitely non-standard (well, there are a lot of things non-standard compared to upstream in this role anyway...like deploying using uwsgi...).

[lae]

the first scenario, in more concrete (pseudocode) terms, with a loop to help dedupe tasks:

- block:
    - name: collect list of files in shared custom modules directory
    - name: copy validator modules into shared custom modules directory
    - name: symlink custom modules into current directory
    # above two looping against files
    - name: prune validator modules from shared custom modules directory
    - name: prune custom module symlinks from current directory
    # above two looping against the glob from the first task and checking against files
  loop:
    - folder: custom_validators
      files: "{{ netbox_custom_validators }}"
    - folder: custom_pipelines
      files: "{{ netbox_custom_pipelines }}"

also, because those folders are our own folders (the entire shared folder technically is a convention of this role, not of NetBox) then we're well within our rights to say "if you manually put files in here then you should expect them to be removed if they're not defined in your Ansible deployment. So the above set of tasks would also kind of make the _enabled variable redundant. If there were other configuration steps (like installing dependencies) then it would make sense to have that toggle.

the second scenario sounds like it might simplify it a bit, no need for the symlink tasks and you avoid having orphaned symlinks when the current directory changes due to an update, and you'd just add the PYTHONPATH hack to the configuration.py (which can just always be a part of the template because it effectively does nothing if the folders are empty, but adding a conditional based on whether or not the role variables are empty would also be fine).

[sol1-matt]

I'll have a dig around and see if the symlink method works for git-based installs.
If not I'll see what Netbox needs in terms of configuration for installing to venv.

Those 2 things will drive what happens with the files.