PR-2930 add tests #169
Merged
PR-2930 add tests #169
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
reweeden
commented
Jan 22, 2022
reweeden
force-pushed
the
rew/pr-2930-add-tests
branch
from
e067ba8 to
70940c1
Compare
Jlrine2
reviewed
Jan 25, 2022
benbart
reviewed
Jan 27, 2022
| }, | ||
| { | ||
| "regex": r"(EDL-[A-Za-z0-9]+)[A-Za-z0-9]{40}([A-Za-z0-9]{10})", | ||
| "replace": "\\g<1>XXX<EDLTOKEN>XXX\\g<2>", |
There was a problem hiding this comment.
@bbuechler Did you ever need to look for a particular credential in the logs?
reweeden
force-pushed
the
rew/pr-2930-add-tests
branch
from
c95ece9 to
3c59d6a
Compare
benbart
reviewed
Jan 27, 2022
reweeden
force-pushed
the
rew/pr-2930-add-tests
branch
3 times, most recently
from
6f2edc2 to
2bf4b6a
Compare
reweeden
force-pushed
the
rew/pr-2930-add-tests
branch
3 times, most recently
from
61e8535 to
eb5a3d2
Compare
benbart
reviewed
Jan 31, 2022
| @@ -0,0 +1,261 @@ | |||
| import json | |||
| import logging | |||
There was a problem hiding this comment.
Can we rename logging.py to something that doesn't potentially conflict with the standard one?
There was a problem hiding this comment.
What would you suggest? I purposefully used the same name so that it would be clear that these are extensions to the standard logging module, and it's already in the rain_api_core namespace so there shouldn't be any conflicts.
reweeden
commented
Jan 31, 2022
benbart
reviewed
Feb 1, 2022
reweeden
force-pushed
the
rew/pr-2930-add-tests
branch
from
eb5a3d2 to
c7657e9
Compare
reweeden
force-pushed
the
rew/pr-2930-add-tests
branch
from
864b2d2 to
3e4fd27
Compare
reweeden
commented
Feb 1, 2022
Comment on lines
+1
to
+6
| boto3==1.20.29 | ||
| hypothesis==6.36.0 | ||
| moto==2.3.1 | ||
| pytest-cov==3.0.0 | ||
| pytest-mock==3.6.1 | ||
| pytest==6.2.5 |
There was a problem hiding this comment.
Not sure if we need to be pinning the dev dependencies or not
benbart
approved these changes
Feb 4, 2022
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.
Closes #162
Closes #164
I rewrote the JSON logger to always output valid JSON. It would previously only output valid JSON if you passed in a json-like object into the log call such as
log.info({"foo": "bar"}). I suspect that the quote replacement was there because when python print's dictionaries/strings it prints them in a way that looks very similar to JSON except that it uses single quotes (and of course there is no conversion of dictionary keys to strings which is required by the JSON spec) and this was being used as a budget JSON encoder.I changed some of the log censoring, but it might be better to just skip testing that for now and come back to it when we figure out how we want to handle it properly #163.