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

Whitelisting #2

Merged
merged 26 commits into from
Dec 9, 2016
Merged

Whitelisting #2

merged 26 commits into from
Dec 9, 2016

Conversation

alxberardi
Copy link
Contributor

Based on #1

@coveralls
Copy link

coveralls commented Dec 8, 2016

Coverage Status

Changes Unknown when pulling e66c9a8 on whitelisting into ** on master**.

blaknite and others added 6 commits December 9, 2016 10:55
This allows an EnvParser to mask iteslf which reduces object instantiations and simplifies the abstraction. As a result ParameterMasker has been removed and EnvParser now calls SensitiveDataFilter::Mask.mask_hash directly.

Minor improvements to consistency of using parentheses.
PrameterScanner.new is immediately followed by a call to sensitive_data? meaning there is no reason to lazy-load the scans. They’re now just done on initialize.
Abstraction can be reduced resulting in a clear definition of the objects concern during initialization.
@coveralls
Copy link

coveralls commented Dec 9, 2016

Coverage Status

Changes Unknown when pulling 40c655c on whitelisting into ** on master**.

def initialize(env)
@original_env_parser = EnvParser.new(env)
@filtered_env_parser = @original_env_parser.copy
@scanner = ParameterScanner.new(@original_env_parser)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is followed by a function call and then a variable assignment this alignment of equals signs puts me off. Very minor but it's a thing.

spec.name = 'sensitive_data_filter'
spec.version = SensitiveDataFilter::VERSION
spec.authors = ['Alessandro Berardi']
spec.email = ['berardialessandro@gmail.com']
Copy link
Contributor

Choose a reason for hiding this comment

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

There was some comment that if the email has support@travellink.com.au we all end up with ownership when this is published. Worth investigation.

@coveralls
Copy link

coveralls commented Dec 9, 2016

Coverage Status

Changes Unknown when pulling 32ba5da on whitelisting into ** on master**.

@@ -0,0 +1,8 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Why all this?

env is not even guaranteed to be in /usr/bin

There is no good reason for using #!/usr/bin/env bash - either your script must have bash, in which case you are assuming it is installed so it is in /bin - or, your script can get by without bash with just Bourne shell - in which case every UNIX will have /bin/sh.

#!/usr/bin/env bash
set -euo pipefail
IFS=$'\n\t'
set -vx
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need verbose and debug ?

@@ -0,0 +1,8 @@
#!/usr/bin/env bash
set -euo pipefail
IFS=$'\n\t'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we overriding IFS to be tab and newline?

@msmirnoff msmirnoff merged commit 2d7ef41 into master Dec 9, 2016
@alxberardi alxberardi deleted the whitelisting branch December 9, 2016 04:16
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

4 participants