-
Notifications
You must be signed in to change notification settings - Fork 63
Add Napalm Getter feature #149
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
Conversation
|
Can you add the optional dependencies, our json schema repo should show how that is done. Need to add tests as well |
|
I think that we need to document how to install the optional dependencies and when to install them |
|
Will work on getting docs updated. |
|
should be ready now. |
.github/workflows/ci.yml
Outdated
| run: "poetry run invoke pylint" | ||
| needs: | ||
| - "build" | ||
| pytest_all: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to separate out, the ci can just run with pytest-all, no? Otherwise, creating more steps in the github actions runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now pytest only runs 1 test, and that is the check to make sure the exception is raised on the get_napalm_getters() when napalm is not installed. pytest-all runs every other test including testing the actual get_napalm_getters() functionality when napalm is installed. I must be missing something on what you're asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like
strategy:
fail-fast: true
matrix:
python-version: ["3.7", "3.8", "3.9", "3.10"]
testing-type: "pytest_all"
include:
- python-version: "3.10"
testing-type: "pytest"
| pydocstyle = "*" | ||
| m2r2 = "*" | ||
| sphinx = "*" | ||
| sphinx-rtd-theme = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can likely remove as well, (missed by me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
along with m2r2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing m2r2 is good, once I remove sphinx-rtd-theme I can no longer build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see initial pipeline failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed m2r2, left sphinx-rtd-theme for now until we determine what is using this dependency and while builds start to fail once its removed.
|
@qduk want to review? |
qduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Add Napalm Getter feature
Add Napalm Getter feature
fix #95