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

Dont log unknown values #82

Merged
merged 3 commits into from Sep 7, 2016
Merged

Dont log unknown values #82

merged 3 commits into from Sep 7, 2016

Conversation

ajm188
Copy link

@ajm188 ajm188 commented Sep 1, 2016

If users of this package happened to forget to register a key that stores sensitive data, that sensitive data would be logged in ConfigNamespace.validate_keys().

This change adds an optional argument to turn on this behavior.
It currently defaults to False, but it may be nice to change this to True, so that all users need to do to enable this feature is to bump their package version, but I am not sure what you think.

@dnephin
Copy link
Owner

dnephin commented Sep 2, 2016

There is already an option to turn this off:

import logging
logging.getLogger('staticconf.config').setLevel(logging.WARN)

@@ -10,6 +10,8 @@

from testing.testifycompat import (
assert_equal,
assert_in,
assert_not_in,
Copy link
Owner

Choose a reason for hiding this comment

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

These are only for backwards compatibility.

Please use raw assert

assert x in y
assert x not in y

Copy link
Author

Choose a reason for hiding this comment

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

Ah okay. Fixing

@ajm188
Copy link
Author

ajm188 commented Sep 2, 2016

logging.getLogger('staticconf.config').setLevel(logging.WARN)

But this will not log if there are unknown keys in the config, since that is done at info level. The problem this aims to solve is logging just the unknown keys at info level, rather than both the unknown keys and the (potentially sensitive) values they map to.

@ajm188
Copy link
Author

ajm188 commented Sep 6, 2016

@dnephin bump :)

@dnephin
Copy link
Owner

dnephin commented Sep 7, 2016

Ok, I'm happy to merge this when the tests are passing. It looks like pip has changed its args. Would you mind removing the old flag from .travis.yml to get CI running again?

@ajm188
Copy link
Author

ajm188 commented Sep 7, 2016

Sure thing

@coveralls
Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage increased (+0.02%) to 95.461% when pulling 12cf39e on ajm188:dont-log-unknown-values into fda08f8 on dnephin:master.

@ajm188
Copy link
Author

ajm188 commented Sep 7, 2016

Hmmm @dnephin it looks like the version of flake8 you're using no longer supports py26? It's trying to use an OrderedDict: https://travis-ci.org/dnephin/PyStaticConfiguration/jobs/158206819#L211-L216

@ajm188
Copy link
Author

ajm188 commented Sep 7, 2016

@dnephin I looked into this, and it seems we have two options:

  1. drop py26 support
  2. pin flake8 < 2.999

I'm good with either; which do you prefer?

@dnephin
Copy link
Owner

dnephin commented Sep 7, 2016

Ok, I pushed #83, if you rebase you should be set.

@ajm188
Copy link
Author

ajm188 commented Sep 7, 2016

Oh cool. Rebasing away

Andrew Mason added 3 commits September 7, 2016 09:22
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.596% when pulling 610cdd2 on ajm188:dont-log-unknown-values into aaba3ef on dnephin:master.

@dnephin
Copy link
Owner

dnephin commented Sep 7, 2016

LGTM

@dnephin dnephin merged commit 9e6d6f2 into dnephin:master Sep 7, 2016
@ajm188
Copy link
Author

ajm188 commented Sep 8, 2016

Hey @dnephin are you going to cut a pypi release with this change?

@ajm188 ajm188 deleted the dont-log-unknown-values branch September 8, 2016 20:27
@dnephin
Copy link
Owner

dnephin commented Sep 8, 2016

@ajm188
Copy link
Author

ajm188 commented Sep 8, 2016

Awesome, thanks!

@dnephin
Copy link
Owner

dnephin commented Sep 8, 2016

Thanks for the contribution!

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.

None yet

3 participants