disable recording by default #305
Merged
Conversation
This file contains 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
apotterri
force-pushed
the
v2_20240512
branch
11 times, most recently
from
c8b5dda
to
b4c4ba7
Compare
Make sure `APPMAP_RECORD_REQUESTS=true` will generate request recordings when running tests. Previously, it was ignored, and request recordings were never generated when tests were run.
Combine APPMAP_RECORD_PYTEST and APPMAP_RECORD_UNITTEST into APPMAP_RECORD_TESTS.
dividedmind
requested changes
May 23, 2024
There was a problem hiding this comment.
Looks mostly ok, though explicitly setting APPMAP=false (or anything else than true) should not cause a warning. (BTW, I noticed that with the previous version APPMAP=false doesn't seem to do anything at all — warning is still displayed and appmaps are still recorded.)
docs/recording-env-vars.md
Outdated
| @@ -3,11 +3,11 @@ recording types. In each case, ✓ means that the corresponding recording type | |||
| will be produced, ❌ means that it will not. | |||
|
|
|||
| ## Web Apps | |||
| These tables describe how `APPMAP_RECORD_REQUEST` and `APPMAP_RECORD_REMOTE` are | |||
| These tables describe how `APPMAP_RECORD_REQUESTS and `APPMAP_RECORD_REMOTE` are | |||
There was a problem hiding this comment.
Missed a backtick here.
appmap/__init__.py
Outdated
| def enabled(): | ||
| return Env.current.enabled | ||
| else: | ||
| print(_DISABLED_BY_DEFAULT_MSG, file=sys.stderr) |
There was a problem hiding this comment.
Won't this cause the warning to be displayed even if the user explicitly sets APPMAP=false?
This seems very annoying.
There was a problem hiding this comment.
Yeah, I noticed this was wrong right before I read your comment. I adjusted the indentation of the else, should work properly now.
Also, fix a typo.
Recording is no longer enabled by default. It is now necessary to run scripts with appmap-python, or explicitly set APPMAP=true BREAKING CHANGE: disable record by default
It doesn't make sense (and doesn't work) to try to capture other recording types when the user says they want a process recording (by setting APPMAP_RECORD_PROCESS=true).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The agent no longer records by default. Code must be run with
appmap-python, or withAPPMAP=trueto create AppMap data files.Additionally,
APPMAP_RECORD_PYTESTandAPPMAP_RECORD_UNITTESThave been replaced byAPPMAP_RECORD_TESTS. Also,APPMAP_RECORD_PROCESSnow disables other recording types.Fixes #299.
Fixes #300.
Fixes #301.
Fixes #307.