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.

Sign up for GitHub

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

Jump to bottom

Swap ptvsd for debugpy, ptvsd deprecated #4115 #4322

Merged
merged 17 commits into from
May 10, 2021
Merged

Swap ptvsd for debugpy, ptvsd deprecated #4115 #4322

merged 17 commits into from
May 10, 2021

Conversation

yilmi
Copy link
Contributor

@yilmi yilmi commented Apr 20, 2021 •
edited by valentijnscholten
Loading

Trying to move to debugpy in replacement of ptvsd. fixes #4115
Changed a few things:

  • Removed the ptvsd profile (setEnv.sh, compose file, entrypoint script and other references)
  • Added support for two new environment variables:
    • DD_DEBUG_PORT to provide a custom port number for the debugging server
    • DD_DEBUG_WAIT_FOR_CLIENT pauses the execution until the debugging client is connected (doesn't work in multiprocess or multithreaded with uwsgi)

Tested debugging, managed to consistently hit breakpoints, inspect variables, etc...

@madchap madchap self-requested a review April 20, 2021 11:48
@madchap
Copy link
Collaborator

madchap commented Apr 20, 2021

From my previous config, I simply:

  • Rebuilt the image to get the new dep
  • Relinked from ptvsd et dev
    --> and I could attach to debugpy and breakpoint with no further tweaks 👍

My VScode setup remained identical.

Thanks!

@madchap madchap self-requested a review April 20, 2021 15:26
@madchap madchap requested a review from a team April 20, 2021 15:27
@valentijnscholten
Copy link
Member

I think users should be able to disable debugging if they don't want to use it for whatever reason (performance, overhead, ...)
And as such I think we should still support running more than 1 process/thread for those who want to test things related to multi threading.

@madchap
Copy link
Collaborator

madchap commented Apr 20, 2021

Yea, I think this was the reason we kept the 2 separate for a long time.

Might need something fancier to handle variables and keep it flexible.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@madchap madchap self-requested a review April 30, 2021 13:01
madchap
madchap requested changes Apr 30, 2021
View reviewed changes
Copy link
Collaborator

@madchap madchap left a comment

Choose a reason for hiding this comment

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

As @valentijnscholten mentioned, we'd need a way to control whether or not we want debugpy active or not and based on that, be able to run multi-process and multi-thread (which indeed is troublesome when wanting to attach a debugger).

@yilmi yilmi force-pushed the debugpy_dojo_4115 branch 2 times, most recently from a1e951c to 93db55b Compare May 6, 2021 15:37
@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2021

Conflicts have been resolved. A maintainer will review the pull request shortly.

@yilmi
Copy link
Contributor Author

yilmi commented May 6, 2021 •
edited
Loading

I think users should be able to disable debugging if they don't want to use it for whatever reason (performance, overhead, ...)
And as such I think we should still support running more than 1 process/thread for those who want to test things related to multi threading.

@valentijnscholten I changed the dev entrypoint to override the default if DD_DEBUG is set. I also added back the debug profile (with its docker-compose override)

@madchap
Copy link
Collaborator

madchap commented May 7, 2021

I'll give this a spin in the following days. Looks good at first sight though.

@madchap madchap closed this May 7, 2021
@madchap madchap reopened this May 7, 2021
madchap
madchap previously requested changes May 7, 2021
View reviewed changes
Copy link
Collaborator

@madchap madchap left a comment

Choose a reason for hiding this comment

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

Please bump the version with a minor in Chart.yml to pass the linting.
To be on the safe side, quickly rebase too.

Cheers :)

requirements.txt Outdated Show resolved Hide resolved
damiencarol
damiencarol requested changes May 7, 2021
View reviewed changes
Copy link
Contributor

@damiencarol damiencarol left a comment

Choose a reason for hiding this comment

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

Made some minor rename comment and also pin the version for debugpy

helm/defectdojo/values.yaml Show resolved Hide resolved
helm/defectdojo/templates/django-deployment.yaml Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@madchap madchap closed this May 7, 2021
@madchap madchap reopened this May 7, 2021
@madchap madchap closed this May 10, 2021
@madchap madchap reopened this May 10, 2021
@valentijnscholten
Copy link
Member

@madchap it's better to only rerun the failed tests instead of closing/reopening and running all tests again.

Yassine Ilmi and others added 16 commits May 10, 2021 13:21
@valentijnscholten valentijnscholten dismissed madchap’s stale review May 10, 2021 12:57

as discussed on slack need to dismiss this to be able to merge

@valentijnscholten valentijnscholten merged commit 591e1be into DefectDojo:dev May 10, 2021
@yilmi yilmi deleted the debugpy_dojo_4115 branch May 10, 2021 13:08
dsever pushed a commit to dsever/django-DefectDojo that referenced this pull request May 18, 2021
@yilmi @damiencarol @dsever
Swap ptvsd for debugpy, ptvsd deprecated DefectDojo#4115 (DefectDojo#…
b61bbe1 
…4322)

* Merging ptvsd into dev profile

* Documentation minor update and script cleanup

* Updating helm charts to remove mentioned of ptvsd

* Updating docker documentation

* Adding comments in code

* pep8 fixes

* Update docker-compose dev to get port from env

* Keeping debug profile, with same entrypoint as dev

* Documentation adjustments

* Setting variable in helmchart to enable debug

* Final docs changes

* Update helm/defectdojo/templates/django-deployment.yaml

Co-authored-by: Damien Carol <damien.carol@gmail.com>

* Update requirements.txt

Co-authored-by: Damien Carol <damien.carol@gmail.com>

* Bump chart version, and rename value

* Rebasing from dev

* pep8 fixes

* Rebasing

Co-authored-by: Damien Carol <damien.carol@gmail.com>
madchap pushed a commit to madchap/django-DefectDojo that referenced this pull request May 30, 2023
@yilmi @damiencarol @madchap
Swap ptvsd for debugpy, ptvsd deprecated DefectDojo#4115 (DefectDojo#…
1e8a644 
…4322)

* Merging ptvsd into dev profile

* Documentation minor update and script cleanup

* Updating helm charts to remove mentioned of ptvsd

* Updating docker documentation

* Adding comments in code

* pep8 fixes

* Update docker-compose dev to get port from env

* Keeping debug profile, with same entrypoint as dev

* Documentation adjustments

* Setting variable in helmchart to enable debug

* Final docs changes

* Update helm/defectdojo/templates/django-deployment.yaml

Co-authored-by: Damien Carol <damien.carol@gmail.com>

* Update requirements.txt

Co-authored-by: Damien Carol <damien.carol@gmail.com>

* Bump chart version, and rename value

* Rebasing from dev

* pep8 fixes

* Rebasing

Co-authored-by: Damien Carol <damien.carol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damiencarol damiencarol damiencarol approved these changes

@valentijnscholten valentijnscholten valentijnscholten approved these changes

@madchap madchap madchap left review comments

Assignees
No one assigned
Labels
enhancement maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yilmi @madchap @valentijnscholten @damiencarol