fix(nodes): respect nodes denylist #5893
Conversation
In #5838 graph validation was updated to resolve the issue with the nodes union and import order. That broke the nodes denylist functionality. However, because the corresponding test was marked as `xfail`, we didn't catch the issue. - Fix the nodes denylist handling - Update the tests
psychedelicious
requested review from
blessedcoolant,
brandonrising and
hipsterusername
as code owners
github-actions
bot
added
python
|
I think the test failure is related to how pytest scopes test runs. I'll pick this up later. |
We don't need separate implementations for this class, let's not complicate it with an ABC
TypeVar is for generics, but the usage here is as an alias
The only pydantic usage was to convert strings to `Version` objects. The reason to do this conversion was to allow the register decorator to accept strings. MigrationEntry is only created inside this class, so we can just create versions from each migration when instantiating MigrationEntry instead. Also, pydantic doesn't provide runtime time checking for arbitrary classes like Version, so we don't get any real benefit.
This was checking a `Version` object against a `MigrationEntry`, but what we want is to check the version object against `MigrationEntry.from_version`
|
I have fixed the "test_deny_nodes()" test so that it doesn't interfere with later tests. The fix involved clearing the config cache after the test finishes running so that the Both cache clearing steps are done in a context manager, and should be immune to exceptions, etc. |
|
Thanks! It looks like this picked up the config migrator though, need to wait for that to merge before this PR can merge. Or bring the changes in this PR into the config migrator PR. |
Sorry - wonder how that happened? |
What type of PR is this? (check all applicable)
Description
In #5838 graph validation was updated to resolve the issue with the nodes union and import order. That broke the nodes denylist functionality.
However, because the corresponding test was marked as
xfail, we didn't catch the issue.QA Instructions, Screenshots, Recordings
The test coverage is sufficient.
Merge Plan
This PR can be merged when approved
Added/updated tests?
have not been included