Include a client_id in the results from ProfileFetcher #66
Conversation
|
@mlopatka @birdsarah r? Coveralls hates my logging. |
|
Crap. I need to add microbenchmarks and some instructions on running the test_integration script. |
from local development
crankycoder
force-pushed
the
features/issue-65
branch
from
fc53aa6
to
f5d5a0d
Compare
| return rm | ||
|
|
||
|
|
||
| @pytest.mark.skip("This is an integration test") |
There was a problem hiding this comment.
Can you explain your thinking here around writing a new test but skipping it.
If we don't want to run it on CI, we can skip it there.
If we don't want to run it by default we can add a pytest mark and update setup.cfg to exclude that mark by default.
I'm concerned that this test will never be run and will bitrot.
There was a problem hiding this comment.
The function here is mostly a smoke test. We've had problems where we missed something that unit tests could not adequately capture. To run an end-to-end test, a person needs to have proper S3 credentials to read the JSON blobs from the dev environment.
bitrot is a real concern - we can have ops provision a special CI access key for us to read the JSON blobs for each of the recommenders when we run under test.
@mlopatka - i don't imagine there's a privacy concern here, but is that ok?
Basically - anyone who uses our CI access key would have read permission on each of the JSON blobs for the recommenders.
There was a problem hiding this comment.
okay. lets open an issue. there should be a way to make this work. i've worked with similar problems before - e.g. you only want certain committers to be able to upload to your s3 bucket.
Logging useful information from the ProfileFetcher was unnecessarily difficult because we were missing a
client_idin the result output.This patch adds the client_id attribute into the dictionary result from ProfileFetcher::get(client_id), and updates the related testcase.
I've thrown in an update to the README.md which also closes off #29.