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

Migrate to pydantic #214

Draft
wants to merge 88 commits into
base: master
Choose a base branch
from
Draft

Migrate to pydantic #214

wants to merge 88 commits into from

Conversation

terjekv
Copy link
Collaborator

@terjekv terjekv commented Apr 9, 2024

Another large scale rewrite to move away from a bundle of dicts into something actually structured into Pydantic models. The goal here is to be able to add features and output data without needing to test, guess, and generally fiddle with dicts everywhere...

  - Also makes get_list do the logic of checking hit limits and exception handling.
  - Only the API module has any idea of endpoint addresses for pydantic calls. The rest of the client code does not care.
  - Implements a IPAddressField for Hosts to reduce the number of str-fields we have. We should ideally have none.
@terjekv terjekv self-assigned this Apr 9, 2024
terjekv and others added 8 commits April 9, 2024 15:07
* Clean up authentication mess.

  - Cleans up the names for the auth functions.
  - Adds a LoginFailedError exception.
  - Passes on params from the pre-relogin command to the post-login command.
  - Reintroduces the previous behaviour where failing the login upon startup exists mreg.
* Allow oneshot commands from the terminal.

This patch allows the use of mreg-cli without interaction, ala:

`mreg-cli host info foo`

This will then feed the command `host info foo` to the cli and then exit.

Also adds an option to force token only login. Good for scripts. :)
- No micromanagment of error types, just dump the error text back to the user.
## The problem...

It was much too easy to do `host remove foo -f` and have it bite in the worst possible way.

## The solution?

In this PR, `-force` is no longer a catch-all for making `host remove`, well, remove the host. If the host has cnames, ptrs, mx, srvs or ipadresses across multiple vlans, the user needs to declare a desire to override each of these explicitly via the new option `-override`.

## Supported input

Accepted overrides are `cname`, `ipadress`, `mx`, `srv`, `ptr`, `naptr`. Invalid overrides offered as parameters to `-override` are errors and the command will report on the unexpected input and stop before executing anything.

The choice to have these as textual inputs is intended with the explicit goal of making their use require more than tab-completing an option.

## Example usage

`host remove foo -force -override cname,ipaddress,mx`.

This would allow deletion / removal of a host with cnames, ipadresses across different vlans, and mx set. However, any ptr or srv RRs will still cause the deletion to cancel.

## Example warning from `host remove`

```
WARNING: : bar.example.org requires force and cnames as overrides for deletion:
  1 cnames, override with 'cname'
    - fubar.example.org
  multiple ipaddresses on the same VLAN. Must use 'force'."
```

## Notes:

1. `-force` alone works on multiple ipadresses from the same VLAN.
2. `-override` requires the presence of `-force`. The documentation states this both from expanded inline help and `host remove -h`.


---------

Co-authored-by: pederhan <[email protected]>
@pederhan
Copy link
Member

pederhan commented Apr 9, 2024

I've used Pydantic for a few CLI projects by now, and there are a couple of points I think you should have in the back of your mind when developing this further:

  1. How will you handle data that fails validation? When fetching all hosts, should a single failing host break the entire host fetching functionality of the application, or will you implement some sort of best-effort fetching?
  2. When validation fails, how do you notify the user about it? Do you leak the entire stack trace, show just the Pydantic ValidationError message, or write some custom exception message or handler that hides it from the user entirely?

Regarding point 2, it's important to address this IMO, as the ValidationError exceptions provide comprehensive information about what and why validation failed, but it's not really something the user can act on, so to them it's kind of meaningless noise.

FWIW, I don't have good answers to these questions. You want to balance reliability and ease of use with correctness and debuggability, and those factors can be at odds with each other at times.

@terjekv
Copy link
Collaborator Author

terjekv commented Apr 9, 2024

Yeah, I've been down the same thoughts myself. Tbh, these kinds of problems are why I'm looking at gRPC for other projects, and also why they tend to come in Rust. Which doesn't really help the here and now. :p

terjekv and others added 16 commits April 9, 2024 22:08
  - Also differentiates between get_list and get_list_unique where one expects many or one hit.
  - Further improve models with better cleaning of macaddress.
  - Avoid string literals for endpoints, use an Enum with optional identifier application.
  - api.get_host now allows for lookups by id, ipaddress, macaddress or hostname, and if all that fails checks for CNAMEs.
  - Add network endpoints and more.
  - Make models frozen so they can be used as hash keys, returning new objects of one tries to assign to them via acceptable methods. __setattr__ and __delattr__ are overridden to cause AttributeErrors if used.
  -  Migrate `host add` and `host remove`
  - Document (some) of the older API calls with exceptions.
  - Host output now uses class methods from the models involved to print multiple entries.
  - clean_hostname is copied around a bit to support old and new code... :(
  - Models with a host parameter also get a method to resolve the host.
  - Added support for created_at and updated_at.
  - We don't use clean_hostname, we have a hostname type.
  - Make roles look sane for the consumer of the cli.
  - Models know their endpoint
  - This allows for class.get, class.create, obj.delete, obj.refetch, obj.patch
  - patch/create return the new obj
  - Needs refactoring of model file, it's getting large.
  - This does not rely on getting the IP from the server first, it uses the servers API directly.
mreg_cli/utilities/api.py Outdated Show resolved Hide resolved
terjekv and others added 15 commits May 14, 2024 14:21
* Labels and some permission work.
* Allow create not to fetch, as Label.create() will... fail...
* Add missing `cli_info` calls, use `print_msg=`

---------

Co-authored-by: pederhan <[email protected]>
* Permission commands migrated.

---------

Co-authored-by: pederhan <[email protected]>
* Zone commands

* Use illegal hack to create zones

* Re-add zone_basepath for development

Why was it gone?

* Delegation abomination

* Make `with_*` methods available outside `Endpoint`

* Get/create/delete delegations

* Add Zone.get_delegation_{and,or}_raise`

Really re-inventing the wheel here just because the endpoint has 2 component placeholders...

* Remove WithName mixin from `Delegation`

* Add `zone list`

* Move `with_*` methods back into `Endpoints`

* Add `zone set_ns`

* Fix `ReverseZoneDelegation.is_reverse()`

* Remove unused Delegation.type_by_name

* Replace `WithName` with `APIMixin`in `Zone`

* Fix invalid imports

* Add `zone info`

* Fix incorrect endpoint values, reorder endpoints

* Require force for `delegation_create` if zone does not exist

* Add `Zone.get_zone_{and,or}_raise`

* Simplify Zone.delete_delegation

* Add `delegation_list`

* Add `zone set_soa`

* Add `zone delegation_comment_set`

* Add `zone delegation_comment_remove`

* Add `zone set_default_ttl`

* Improve TTL printing

* Remove unused zone functions

* Change `output_ttl` param order for consistency

* Use `get_by_name*` instead of `get_by_field*`

* Remove unused imports

* Comments, docstrings, CLI messages
* Support segmenting tokens per user and url.

* Clean up debugging code.

* Exit after showing token, allowing for scripted use.

* Only print the token itself, not the Token header prefix.

* Add utility functions for setting/getting auth header (#231)

* Revert line breaks in overload definitions.

---------

Co-authored-by: Peder Hovdan Andresen <[email protected]>
* Set prompt to default show the name of the server.

* Update documentation on defaults.
* Remove `use_json` (#235)

* Add `WithHistory`, remove history methods from `APIMixin`

* Access attribute directly
* Add more flexible JSON types

* Add `ExcludedRange` model type

I was not able to find a single network with an excluded range, so it's hard to test...

* Allow `max_hits_to_allow=None`

* Add `network create`

* Add `network info`

* Add `network find`

* Make JSON type names less verbose

* Handle non-str args in `NetworkOrIP` validator

* Refactor Network `get_*` methods

* Add `network find -ip` support

* Support `limit=None` in `APIMixin.get_by_query`

* Raise `InputFailure` in `IPAddressField` validator

* Only support IP args with`network find -ip`

* Add `network list_unused_addresses`

* Add `network list_used_addresses`

* Add `network remove`

* Replace `cli_warning` calls with exceptions

* Add `network add_excluded_range`

* Add `network remove_excluded_range`

* Add new command `network list_excluded_ranges`

* Add `network {set/unset}_*` commands

* Handle location and category tags from config

* Fix sample config tags

* Update help text of `network set_reserved`
* Add pagination support for `get_typed`

* Fix missing param in docstring

* Handle non-paginated results in `get_list`
* Migrate fully to pyproject.toml

* Add dev dependencies

* Fix whitespace

* Compile requirements files from pyproject
* Fix wrong args attribute name in `naptr_show`
* Tests: Use `label set_description`
* Fix `dhcp assoc`

* Fix `dhcp disassoc`

* Refactor `dhcp` commands

* Remove redundant name binding

* Improve `ipaddress_from_ip_arg()` exceptions

* Improve `ipaddress_from_ip_arg` docstring
pederhan and others added 11 commits May 29, 2024 11:05
* Validate responses with Pydantic

* Specify part of statement `type: ignore` applies to

* Replace all `get_list` calls with `get_typed`

* Use `TypeAliasType` from typing-extensions

* Revert accidental find and replace change

* HACK: Use `cast()` in `get_list_unique()`

* Add source url comment to recursive JSON type

* Add helper functions for response validation

* Remove `ResponseLike`

* Raise MREG ValidationError on failed validation

* Validate response in `get_list_unique`

* Improve exception handling for failed validation

* Add `HistoryResource` from #232

Also directly validates results to `HistoryItem` instead of going through a dict.
* Strip None values when sending JSON

* Recursively strip
* Require SOCKS dependencies

* Add pyinstaller dev dependency
* Print git commit in `--version`

* Use versioneer to determine git revision

* Use `setuptools_scm`

* Fix scm version

* Fully integrate dynamic versioning

* Delete versioneer-created gitattributes file
- This PR allows for copying of all policies (roles) from one host to another via a new command, `policy host_copy`.
* Support multiple destinations in host_copy.

  - Also reduces output noise.
  - Also handles the situation where a destination already has the role in question.
  - Requires Role to be hashable.
* Publishing

* Add branch condition to publish workflow

* Wording

* Remove usage of requirements.txt files

* Add basepython resolution for numbered envs

* Use uv in tox workflows

* Unfinished publish workflow

* trying stuff

* Do not specify tox env in build workflow

* Move pyinstaller dependency to tox

* Actually run tox

* Fix binary dist path

* Revert incorrect binary path

* Fix underscore in artifact name

* verbose pypi

* Use test pypi

* Use `mreg-cli-v*` tag pattern

* Dump workflow info

* Add branch check

* Try to create release

* Try to use trusted publishing

* Remove unused pypi vars

* Try to rename binaries

* Try to actually read actions docs

* Fix bin  name

* Publish a4

* Rename binary after creation

* Remove publishing branch

* Make built binary executable

* Remove executable step

* Build on master and publish to real pypi

* Update publishing instructions

* Exit explicitly only on master branch

* Add artifact and version info, move `-desc`to Removed

* Final publishing test run

* Enable master protection again

* Make GitHub release non-prelease

* Add missing newline

* Fix CONTRIBUTING markdown lint violations

Does not "fix" the ordered list rule violation, since indenting the code blocks looks very awkward.
* Add authors to pyproject.toml

* Add historical authors

* Add AUTHORS file
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.

2 participants