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

Flake8, black, new API endpoints #89

Merged
merged 2 commits into from
Jun 5, 2020
Merged

Conversation

eshaan7
Copy link
Member

@eshaan7 eshaan7 commented Jun 3, 2020

Major Changes:

  • The file api_app/views.py has been deleted in favor of,
    • new api_app/gui.py which includes the routes used by the django templates and,
    • new api_app/api.py which includes the API endpoints used by the official pyintelowl CLI client and the official IntelOwl-ng web client.
  • The API endpoints (ones which exist after /api/) from intel_owl/urls.py are now in the new api_app/urls.py and referenced in the former. This IMHO increases modularity and code maintainability. More info here.
  • The route query_database_json has been removed since we are now using JobViewSet API viewclass feature by Django-rest-framework which provides us automatic routing and serialization for doing operations on the Job Model. More info here. I've updated the href link in the templates/query_database.html to use this new route.
  • New TagViewSet API class in api_app/api.py to provide automatic routing and CRUD operations on Tags Model. (This is used by the IntelOwl-ng client).
  • New obtain_user_token, get_user_info and perform_logout APIs which closes Authentication Service (DRF) #80. These routes are for authenticating and de-authenticating user and their tokens.
  • Added django-cors-headers package to deal with preflight CORS (OPTIONS) request and provide strict access control/ origins whitelisting.

Minor Changes:

  • Little code cleanup in api_app/utilities.py (diff).
  • Fixed some typos in README.md and added information about IntelOwl-ng under "features" heading.
  • All files (except the files under api_app/script_analyzers directory) are updated to replace str.format() with f-strings format. (f-strings everywhere #83)
  • Updated the function doc-strings to match Sphinx/reST syntax.

Changes related to linting:

  • Now using flake8 and black for code formatting and styling. (closes Using black and flake8 effectively #81)
    • Their latest versions are added to requirements.txt
    • Commands for both have been added to .travis.yml,
    • Updated CONTRIBUTE.md with information regarding linting and code formatting.
    • Added the code-style: black badge to README.md
  • All files have been updated to conform with the new psf/black and flake8 rules except the files under api_app/script_analyzers directory. The reason for this being, though we can directly run black and it will lint those files, black can sometimes write breaking changes so we should test each file one by one to be on the safer side.

TESTS:

  • Lint tests. report.
  • Example run with pyintelowl. report.
  • Also works elegantly with IntelOwl-ng.

@eshaan7 eshaan7 added the linting label Jun 3, 2020
@eshaan7 eshaan7 force-pushed the dev-flake8-black-api-auth branch from 1a20f27 to 65b1acc Compare June 3, 2020 16:06
@eshaan7 eshaan7 requested a review from mlodic June 3, 2020 16:27
Copy link
Member

@mlodic mlodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR.

I have just one question. I noticed that you explained why you did not use black in the analyzers code. Did you try to apply it and you found some problems? I understand that it could bring some issues but I think that the testing suite can help us to avoid errors. Can you give it a chance by changing these files too and running the tests locally? In this way the code will be uniform everywhere and we can force little contributors to follow style rules (writing a new analyzer is fast and easy. It is difficult that the main parts of the application will be changed by external people while analyzers are a good target for first contributions). What do you think about?

@eshaan7
Copy link
Member Author

eshaan7 commented Jun 3, 2020

Thanks for the quick review. I did try to use black for the analyzers code, the problem is not with just black but the fact that we have to make 3 new changes in each analyzer file, sequentially: replace all str.format() with f-strings, then enforce black and finally fix leftover flake8 alerts.
So I started doing this and after a few files, it got really tiresome and after a while I started making mistakes (that's why I mentioned that we should test each file) and (maybe it was my laziness) I just thought maybe there's a better way of doing this ?

Maybe we could just enforce black for now and leave f-strings and flake8 issues (only if there are too many of them), but then later when we do switch to f-strings we would need to enforce black again.

@eshaan7
Copy link
Member Author

eshaan7 commented Jun 3, 2020

Can you give it a chance by changing these files too and running the tests locally?

Example:

  1. Ran black to format all files including script_analyzers/*

Output

  1. Then ran flake8, and got 46 alerts under script_analyzers/*:

Output

This is also because black isn't always able to restrict the lines to 88 characters. (related GitHub issue)

@mlodic
Copy link
Member

mlodic commented Jun 4, 2020

Maybe we could just enforce black for now and leave f-strings and flake8 issues (only if there are too many of them), but then later when we do switch to f-strings we would need to enforce black again.

If you did not see critical problems after this when running tests, we can do it I think.
Maybe we can ignore rule W605 and put a higher limit for E501 just for the script_analyzers. Then the alarms would be reduced and maybe we can fix them faster.
What do you think?

@eshaan7
Copy link
Member Author

eshaan7 commented Jun 4, 2020

Maybe we can ignore rule W605

By convention in python 3.x, a regex string should be prepended with r. I think doing this will turn off the alerts and shouldn't take so long. More info on this Stack Overflow thread.

So, after making the requested changes, should I squash my new commits into the previous commit only or should I make these changes in a new commit ?

@eshaan7 eshaan7 force-pushed the dev-flake8-black-api-auth branch from 65b1acc to 1c4eacd Compare June 4, 2020 21:11
@mlodic
Copy link
Member

mlodic commented Jun 5, 2020

for me this is ok, thank you

@mlodic mlodic merged commit cdc591a into develop Jun 5, 2020
This was linked to issues Jun 5, 2020
@eshaan7 eshaan7 deleted the dev-flake8-black-api-auth branch June 10, 2020 13:47
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.

Using black and flake8 effectively Authentication Service (DRF)
2 participants