Improve makefile#343
Closed
oshoma wants to merge 14 commits intoAggregate-Intellect:mainfrom
Closed
Conversation
Collaborator
Author
|
Well I guess I can't claim this PR is "small" any more, as it touches a lot of files. But most of the changes are benign, just reformatting from running I decided to create a separate makefile for the Slackapp. @20001LastOrder does this make sense? In other words, when we run I went down this path because I realized there were broken tests in Slackapp, and I could not make them work without separating the two packages a little bit more. |
This was referenced Apr 21, 2024
oshoma
force-pushed
the
improve-make
branch
3 times, most recently
from
11ee103 to
d88dcc3
Compare
Run `poetry self update && poetry install` after picking up this commit. Changes: - Change all make steps to depend on poetry being installed - Add mypy for static type checking, but leave disabled for now - Use more sophisticated lint options - Include Slack app in `make test` - Include Slack tests in `make lint` - Fail tests if coverage drops below 75% mypy gives us static type checking. It triggers many warnings, so I am disabling mypy for now and will create an issue to resolve mypy warnings.
Change code to resolve these lint warnings: - F821: Undefined names - F841: Local variable name is assigned to but never used - F811: Redefinition of unused name from line name
Change code to resolve these warnings: - F811: Redefinition of unused name from line name - W291: Trailing whitespace `make lint` will now warn and fail if files contain trailing whitespace.
flake8 is currently configured to ignore these errors: W503: Line break occurred before a binary operator E501: Line too long (82 > 79 characters) E402: Module level import not at top of file This change makes the behavior consistent for both `make lint` and `poetry run flake8`. Also removing E402 as that warning was already resolved.
Run `poetry self update && poetry install` to pick this up.
Fixes the following Bandit errors seen with `make lint`: 1. Issue: [B104:hardcoded_bind_all_interfaces] Possible binding to all interfaces. Severity: Medium Confidence: Medium CWE: CWE-605 (https://cwe.mitre.org/data/definitions/605.html) More Info: https://bandit.readthedocs.io/en/1.7.8/plugins/b104_hardcoded_bind_all_interfaces.html Location: apps/slackapp/slackapp/bolt_app.py:346:13 Change: suppress the warning with #nosec B104, since our use of host '0.0.0.0' is intentional. Also add a security note. 2. Issue: [B113:request_without_timeout] Requests call without timeout Location: apps/slackapp/slackapp/bolt_app.py:340:23 Location: sherpa_ai/scrape/extract_github_readme.py:75:19 Location: sherpa_ai/utils.py:83:15 Location: sherpa_ai/scrape/extract_github_readme.py:52:19 Location: sherpa_ai/scrape/file_scraper.py:69:19 Change: add timeouts for each of these. Adjustment may be needed. 3. Issue: [B310:blacklist] Audit url open for permitted schemes. Allowing use of file:/ or custom schemes is often unexpected. Severity: Medium Confidence: High CWE: CWE-22 (https://cwe.mitre.org/data/definitions/22.html) More Info: https://bandit.readthedocs.io/en/1.7.8/blacklists/blacklist_calls.html#b310-urllib-urlopen Location: sherpa_ai/tools.py:59:15 Location: sherpa_ai/utils.py:294:12 Change: add logic to ensure URLs are HTTP or HTTPS. Add tests.
Create a Makefile just for Slackapp so we can run `make test` within /sherpa/src/apps/slackapp using Slackapp's own Poetry environment. Remove knowledge of Slackapp from Sherpa's makefile. Change make to install Spacy model dependency. Add a Poetry lock file for Slackapp. Run `poetry install` in the sherpa and slackapp directories to pick up these changes.
Change code to resolve these lint warnings: - F821: Undefined names - F841: Local variable name is assigned to but never used - F811: Redefinition of unused name from line name
Fixes the following Bandit errors seen with `make lint`: 1. Issue: [B104:hardcoded_bind_all_interfaces] Possible binding to all interfaces. Severity: Medium Confidence: Medium CWE: CWE-605 (https://cwe.mitre.org/data/definitions/605.html) More Info: https://bandit.readthedocs.io/en/1.7.8/plugins/b104_hardcoded_bind_all_interfaces.html Location: apps/slackapp/slackapp/bolt_app.py:346:13 Change: suppress the warning with #nosec B104, since our use of host '0.0.0.0' is intentional. Also add a security note. 2. Issue: [B113:request_without_timeout] Requests call without timeout Location: apps/slackapp/slackapp/bolt_app.py:340:23 Location: sherpa_ai/scrape/extract_github_readme.py:75:19 Location: sherpa_ai/utils.py:83:15 Location: sherpa_ai/scrape/extract_github_readme.py:52:19 Location: sherpa_ai/scrape/file_scraper.py:69:19 Change: add timeouts for each of these. Adjustment may be needed. 3. Issue: [B310:blacklist] Audit url open for permitted schemes. Allowing use of file:/ or custom schemes is often unexpected. Severity: Medium Confidence: High CWE: CWE-22 (https://cwe.mitre.org/data/definitions/22.html) More Info: https://bandit.readthedocs.io/en/1.7.8/blacklists/blacklist_calls.html#b310-urllib-urlopen Location: sherpa_ai/tools.py:59:15 Location: sherpa_ai/utils.py:294:12 Change: add logic to ensure URLs are HTTP or HTTPS. Add tests.
Make changes to fix broken tests and enable Slackapp tests to run from `make test` with src/apps/slackapp as the working directory. Give Slackapp a conftest.py of its own. This file is a copy of Sherpa's conftest. Is there a clean way to reference it directly, instead of copying it? Change bolt_app to reference Sherpa's configuration file using a relative file path.
Applies changes from `make format`.
Collaborator
Author
Collaborator
Author
|
replaced by #356 |
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
1 participant
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.
Your checklist for this pull request
Thank you for submitting a pull request! To speed up the review process, please follow this checklist:
make format)/docs)pytest tests(offline mode)Additional steps for code with networking dependencies:
pytest tests --external_api(online mode, making network calls)Description
Improve the Makefile by adding capabilities to
lint,formatandtest.This gives us a more robust set of
makecommands, including:make lintmake testmake testfails if coverage drops below 75%The changes to files other than the makefile are all cosmetic, including removing
superfluous whitespace and adding lint directives. There are no logic changes.
This is another step towards CI/CD. Once we are confident this is working nicely we can change
our github workflow to use
make testand possiblymake lint.Run
poetry self update && poetry installto pick up these changes.