Hot fix to allow filtering via slug - #107
Draft
sergio-gimenez wants to merge 1 commit into
Draft
Conversation
Member
|
Hello, I personally don't like how this is implemented, while it works, it'll pass over failure conditions. I have also noticed a regression thanks to this. I appreciate you pointing this out. I will submit a new PR to better solve this soon and give you credit for the find! |
danner26
added
type: bug
danner26
marked this pull request as draft
September 8, 2023 17:48
mmguero
pushed a commit
to mmguero-dev/Device-Type-Library-Import
that referenced
this pull request
Aug 11, 2026
…x-community#107) * fix: document Docker usage and make a mounted repo volume work The README had no Docker section, so the published image at ghcr.io/marcinpsk/device-type-library-import was not discoverable. Add a chooser at the top of the README for the two supported ways to run the tool, a Docker section (tags, volumes, argument passing, Compose, networking), and an explicit statement that the project is not published to PyPI. Writing those instructions surfaced two defects in the volume flow they describe: - DTLRepo treated any existing directory at REPO_PATH as an existing clone and went down the pull path. Docker always creates the mount point before the container starts, so the first run with a volume at /app/repo failed with `An unknown error occurred: "Git Repository Error"` (GitPython InvalidGitRepositoryError). Test for a .git directory instead. - /app/repo did not exist in the image, so Docker created a named volume mounted there as root and the container, which runs as appuser, was rejected by validate_repo_path with `Invalid repository path "/app/repo"`. Create the directory in the image before the chown so the volume inherits appuser ownership. Closes netbox-community#106 * test: pin the git identity used by the real-Git repo fixture Commits in the fixture fell back to GitPython's generated identity, which comes from getpass.getuser() and the node name. getpass.getuser() raises OSError when the process UID has no passwd entry, which happens in containers run with an arbitrary --user and a stripped environment. Pass a fixed Actor to both commits so the fixture depends on nothing outside the test. * docs: correct the host.docker.internal guidance for Linux Measured on a Linux host with Docker 29.7: without --add-host the name does not resolve at all ("bad address"); with it the name resolves to the bridge gateway, so a NetBox bound only to 127.0.0.1 answers with connection refused rather than being reachable. Only Docker Desktop provides the name for free. Recommend --network host first, since it works regardless of the bind address, and state both Linux constraints instead of presenting the two options as equivalent. * refactor: tidy the Docker volume layer and the real-Git test helper Self-review pass over this PR's own new code, no behaviour change: - Merge the mkdir into the existing useradd/chown RUN so the ownership fix is one layer and the chown cannot drift away from the mkdir. - Return only the exception handler from _init_dtl; the DTLRepo instance was never used by either test. - Move the pinned ACTOR above the fixture and add the missing docstring. Re-verified: red against the pre-fix core/repo.py, green after, green with the git identity unavailable, 948 pass. Named volume still clones on the first run and pulls on the second. * docs: document the export-diff arguments --export-diff, --export-diff-dir, and --force-export-overwrite were in the CLI but absent from the README, so the only way to find them was --help. Add the three rows and an Export Mode section covering the direction of the flow, the default extra/ target, overwrite behaviour, and the import-only flags that are rejected rather than ignored. Each exclusion was checked against the CLI: --update, --only-new, --remove-components, --remove-unmanaged-types, --slugs, --verify-images, and --force-resolve-conflicts all exit with an error, while --vendors is accepted and narrows the export. * test: fail when the README arguments table drifts from the parser The --export-diff flags shipped undocumented because nothing tied the argparse definitions to the README table. Assert both directions against the real parser object: every long option the parser defines has a row, and the table lists no flag the parser has dropped. The check reads the Argument column only. Scanning whole rows let a flag mentioned in another row's description count as documented, which masked a deleted row for --export-diff during verification. * feat: add a container entrypoint so flags can be passed directly docker run … IMAGE --vendors apc now works. Previously the image had no ENTRYPOINT, so the whole command had to be repeated. The entrypoint sends flags, and a bare run, to the importer. Anything else runs as given, which keeps `docker run … bash` and `docker run … python -c …` working for debugging. The pre-entrypoint form is also accepted and its interpreter prefix dropped, for anyone who worked it out from the Dockerfile before the image was documented. Note that this form already survives via the command passthrough, since WORKDIR is /app; the shim makes the guarantee explicit rather than incidental. CI built the image on every push but never ran it, so a broken entrypoint or a root-owned /app/repo would only have shown up for users. Add a smoke-test job that builds once and checks the invocation forms, the passthrough, and the /app/repo ownership, and gate the publish job on it. Verified the script fails on each real breakage: no ENTRYPOINT, a lost executable bit, and a root-owned /app/repo. Also fix two README examples found in review. Docker creates a missing bind-mount source as root, so `-v "$PWD/repo:/app/repo"` failed with `Invalid repository path` until the directory was created first. The export directory resolves to /app/extra in the container, so `--rm` discarded the output unless a host directory was mounted. * fix: let export runs tolerate SLUGS from the environment The --slugs default was settings.SLUGS, so argparse could not tell an explicit flag from an environment default. Export validation rejects a truthy args.slugs, which made SLUGS in an env file fail --export-diff with "--slugs is an import-only flag" even though the user passed no such flag. The README told users to put SLUGS in the env file and reuse it for every Docker mode, so it walked them into that error. --slugs now defaults to None as a "not supplied" sentinel and the environment value is applied after validation. An explicit --slugs is still rejected. Export reports that it ignores an environment SLUGS rather than dropping it silently. Also document that a bind-mount source must be writable by UID 1000, and assert the volume ownership in the smoke test against a real named volume plus a write, not against the image filesystem. * test: restore the lost def header that merged two tests The _run_export_diff assertions ran as the tail of test_validate_argument_combinations_blocks_force_resolve_with_export_diff, behind an orphaned docstring. The coverage was real but unnamed, and it was skipped whenever the preceding assertion failed. Add an AST guard over tests/ for the general case. Ruff cannot catch it: B018 exempts string statements, and no other rule fires. * fix: document digest pinning and cover the export slug clearing The subprocess regression test blanks the NetBox variables, so the run stops at the environment check and never reaches the export. It proved argument validation accepts an environment SLUGS, but not that the value is cleared. Add a test that asserts _run_export_diff receives an empty slug list, red-checked by removing the clearing line. The tag table told readers to pin a version tag, which is not enough here: the image workflow runs on release edited, so an edit rebuilds and re-pushes the same version tag. Document the digest form. * fix: present the bind-mount ownership modes as alternatives The section told the reader to chown the directory to UID 1000 and then offered --user "$(id -u):$(id -g)" as a further option. Doing both leaves a directory owned by 1000 that the container, now running as the host account, cannot write to. Verified: touch fails with Permission denied. They are two modes, so present them that way and say they do not combine. Both are verified end to end against the image, including host-user mode at a non-1000 uid. The export directory follows the same choice, so it no longer hard-codes the chown. The digest example carried a truncated value with a Unicode ellipsis, which Docker cannot resolve. Use a marked placeholder and say which field of the inspect output to substitute. Assert the export warning the test docstring already claimed, so dropping the log line fails the test. * test: assert the exporter receives the parsed export options The test only checked that Exporter was instantiated, so it stayed green if _run_export_diff dropped export_diff_dir, vendors, or force_export_overwrite. Assert the full constructor kwargs instead. Also cover the empty-vendors branch: _run_export_diff maps an empty --vendors to None so the exporter treats it as "all vendors". Nothing pinned that, so passing the empty list straight through went unnoticed. * fix: stop the export comparing NetBox against an absent library Export mode returns before DTLRepo runs, so it never clones or updates the library. The loaders treat a missing directory as an empty one, so every NetBox record fell through to the "absent" branch and was exported. The documented Docker command hit this every time: a fresh container has an empty /app/repo and the example mounted only /app/extra, so an export of the diff silently produced an export of everything. Fail fast instead. run() now requires at least one of device-types, module-types, or rack-types under REPO_PATH, and reports what is missing before any NetBox request. The README says the library is required and the Docker example mounts it. The export tests that used a bare repo directory now create device-types, which is what a real checkout looks like. * fix: treat a worktree or submodule checkout as an existing clone The .git probe added earlier in this branch used isdir(). A worktree or a submodule checkout holds .git as a file, so both read as "no clone here" and git was asked to clone over a populated directory: fatal: destination path '...' already exists and is not an empty directory exists() covers a directory and a file, and still sends a mounted-but-empty volume down the clone path. The tests that forced the pull branch patched os.path.isdir, so they steered nothing once the probe changed. Two of them kept passing while exercising the clone path. They now patch the .git probe alone and leave every other path check real, and a worktree case runs against real git. The real-git tests also clear git's per-command environment variables. A hook runner exports GIT_DIR and GIT_INDEX_FILE for the outer repository, and `git worktree add` inherited them and failed on the wrong index. * test: make the docker smoke test detect a silently broken image Every assertion piped docker into grep, so only grep's status was checked and `set -e` never saw the container fail. A container could print usage and exit 7, or print "unrecognized arguments" and exit 0, and the test still passed. Capture output and status separately and require both. The no-argument check discarded the status with `|| true` and only rejected four Docker error strings. Removing the entrypoint's no-argument branch leaves a bare `exec`, which exits 0 with no output and passed all four. Require the importer's own missing-variable error, which only appears once main() runs. Verified with a stand-in docker that injects one defect at a time. The old script exited 0 for all three; the new one fails each with the reason: noargs-silent old=0 new=1 the no-argument run exited 0, expected 1 help-exit7 old=0 new=1 --help exited 7 unknown-exit0 old=0 new=1 an unknown flag exited 0, expected argparse's 2 The volume name came from $$, which is reused, and `docker volume create` returns an existing volume rather than failing. A stale volume from an interrupted run could be adopted, written to, then force-removed. The daemon now generates the name. Cleanup ran from an EXIT trap alone, which dash does not run on a signal, so cancelling the job leaked the volume. Signalling the running script with TERM leaked it before this change and removes it after, keeping exit 143. * fix: name the two ways out in the absent-library error The message said what was missing but not how to supply it. It now states that export mode does not clone the library, and gives both remedies: clone to REPO_PATH, or run an import, which clones it. The test asserts the remedy is present, not only the missing path. * ci: check the PR title parses as a Conventional Commit A squash merge takes the PR title as the commit subject when the branch holds more than one commit, and semantic-release skips the release when that subject does not parse. The check runs on title edits as well as pushes. The release job installs python-semantic-release unpinned, so the accepted types were whatever the parser defaulted to that day, and "revert" was not among them. pyproject.toml now states allowed_tags, minor_tags, and patch_tags instead of inheriting them. That leaves the same list in two files, so a test compares them. Copying the workflow from another repository verbatim would have accepted "revert" titles that this repository's parser then rejected. * ci: let dependabot derive the commit scope All three ecosystems carried the scope inside the prefix, so the type and scope were one opaque string that had to stay hand-written. Dependabot now appends the scope itself, giving chore(deps) and chore(deps-dev). A prefix that already holds parentheses cannot take include: scope without producing chore(ci)(deps), so the two forms do not mix. The changelog groups by type, not scope, so chore entries land in the same section as before. * test: scan the files pytest collects, not a hand-written glob The hygiene scan globbed test_*.py under tests/. That missed *_test.py, which pytest also collects by default, and it read tests/integration, which norecursedirs excludes from collection. The scan now reads python_files and norecursedirs from pyproject.toml, so it follows the configuration instead of restating part of it. Scan and collection now agree on all 17 files, in both directions. * test: harden the two new config guards Find the PR-title step by its action instead of by position, so adding a step ahead of it fails the lookup rather than reading the wrong step. Note that norecursedirs entries are read as paths, since pytest also accepts globs and this does not expand them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current implementation does not allow filtering by slug. If slug is specified, it returns an error. A quick fix for that is to check that
data['slug]is notNone. If it is, it will end the loop and go for the next iteration.