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

Fixing most of the logging errors + typo and fixes grave security things #438

Closed
wants to merge 4 commits into from
Closed

Conversation

Dhruvacube
Copy link

Pull Request

What does this PR do?

Fixes #338 #337 #434 #312

This PR fixes most of the logging issues, and also I found some security issues like printing of GitHub access token in the console also usage of print function for debugging :) This PR also removes the need of the logging.py file.
Also there was a typo is the README.md file, it also fixes that :)
Also all the python file are formatted with autopep8 .

NOTE: I am coming from Hacktoberfest :)

What part does this affect?

  • FrontEnd.
  • BackEnd.
  • Documentation.
  • Other. (Please specify below)
    README.md

Before submitting

  • Was this discussed/approved via a GitHub issue or slack?
  • Did you read the contributor guideline?
  • Did you ensure that there aren't any other open Pull Requests for the same update/change?
  • Did you make sure the title is self-explanatory and the description concisely explains the PR?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure the code is clean and docstrings have been added or updated as required?
  • Did you make sure the code is linted/formatted locally prior to submission? (using black and/or prettier)
  • Did you make sure to update the documentation with your changes? (if necessary)

PR review

Anyone in the community is free to review the PR once the tests have passed.

Thank you for contributing to AutoDL. We look forward to your continued support.

Copy link
Member

@RusherRG RusherRG left a comment

Choose a reason for hiding this comment

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

Hey @Dhruvacube, thanks for contributing to Auto-DL. I like that you added the necessary log messages at the appropriate place. Although I think you could roll back a few things.

  • Like it was better to have the logging.py since that acts as a global configuration and we then don't need to have logging.basicConfig in each file.
  • The docstrings in files like asgi.py, settings.py, etc. let's keep them as they were.
  • Finally, we are using black for python code formatting so you could use that instead of autopep8

SECURE_HSTS_SECONDS = 31536000
SECURE_HSTS_INCLUDE_SUBDOMAINS = True
SECURE_HSTS_PRELOAD = True
SECURE_REFERRER_POLICY = "same-origin"
Copy link
Member

Choose a reason for hiding this comment

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

I really liked the implementation of the PRODUCTION_SERVER configurations. Thanks ✌️

Copy link
Author

Choose a reason for hiding this comment

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

thanks! I have doing this for many django app, since I read their security docs

@RusherRG
Copy link
Member

Also, it better if you could have separate PRs one for each issue since we have been following that and it makes it easier for us to review the code

@Dhruvacube
Copy link
Author

Hey @Dhruvacube, thanks for contributing to Auto-DL. I like that you added the necessary log messages at the appropriate place. Although I think you could roll back a few things.

  • Like it was better to have the logging.py since that acts as a global configuration and we then don't need to have logging.basicConfig in each file.
  • The docstrings in files like asgi.py, settings.py, etc. let's keep them as they were.
  • Finally, we are using black for python code formatting so you could use that instead of autopep8

ohh okay

@Dhruvacube
Copy link
Author

Dhruvacube commented Oct 19, 2021

Also, it better if you could have separate PRs one for each issue since we have been following that and it makes it easier for us to review the code

I could but you know that unless one pr gets merged I can't make the other one since it will reflect in the existing pr. so thats why I resolved them all into one

@Dhruvacube
Copy link
Author

hey @RusherRG before I pushes the changes I want to ask that do AutoDL wants to log all the log message in a file or is it okay to print them in console??

@RusherRG
Copy link
Member

Also, it better if you could have separate PRs one for each issue since we have been following that and it makes it easier for us to review the code

I could but you know that unless one pr gets merged I can't make the other one since it will reflect in the existing pr. so thats why I resolved them all into one

You can create checkout the main branch into multiple branches on your fork and commit changes corresponding to one particular issue in a branch. Something like the following.

git checkout -b issue-337
git checkout -b issue-338

@Dhruvacube
Copy link
Author

Also, it better if you could have separate PRs one for each issue since we have been following that and it makes it easier for us to review the code

I could but you know that unless one pr gets merged I can't make the other one since it will reflect in the existing pr. so thats why I resolved them all into one

You can create checkout the main branch into multiple branches on your fork and commit changes corresponding to one particular issue in a branch. Something like the following.

git checkout -b issue-337
git checkout -b issue-338

ohhh okay

@RusherRG
Copy link
Member

hey @RusherRG before I pushes the changes I want to ask that do AutoDL wants to log all the log message in a file or is it okay to print them in console??

We need to log the messages to the console as well as the files as it was configured in logging.py. The log configuration is pretty good as it supports rolling logs, colored logs, and outputting to files, hence I was referring to rolling back and using that configuration. So please make that necessary change. Although, we would appreciate it if you have any suggestions for the configuration as I know it needs to be tweaked a bit.

@Dhruvacube
Copy link
Author

Dhruvacube commented Oct 19, 2021

hey @RusherRG before I pushes the changes I want to ask that do AutoDL wants to log all the log message in a file or is it okay to print them in console??

We need to log the messages to the console as well as the files as it was configured in logging.py. The log configuration is pretty good as it supports rolling logs, colored logs, and outputting to files, hence I was referring to rolling back and using that configuration. So please make that necessary change. Although, we would appreciate it if you have any suggestions for the configuration as I know it needs to be tweaked a bit.

hmmm okay!! no problem ! then lemme check some portions of the docs since tbh I don't use logging that much

@Dhruvacube
Copy link
Author

Dhruvacube commented Oct 19, 2021

hey @RusherRG I am adding all the log issues to one pr and then rest in another since, in one commit I did all the logging changes

@Dhruvacube
Copy link
Author

HEY @RusherRG I have created 2 different PR's #444 and #442 according to the review that you gave :)
So you may close this PR and review those two :)

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.

Add log messages in BackEndApp/authv1
2 participants