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

add validation to feature flag key #1278

Merged
merged 1 commit into from Nov 11, 2019

Conversation

@mightyguava
Copy link
Collaborator

mightyguava commented Nov 8, 2019

Only allow things that look like keys to be passed in to FeatureFlags. I'm sure this validation doesn't really belong in misk, but the effort to make this configurable is a bit high and not worthwhile.

I'm also not sure we should have this level of validation. Theoretically users should be testing their code and making sure they are passing in the right key. But, evidence shows that this hasn't been happening. The risk of a bug here is high, since we could potentially launch features to the wrong customers, or worse accidentally send PII to the feature flag vendor when we don't mean to.

So I think this validation is a good bad way of preventing mistakes.

@mightyguava mightyguava force-pushed the yunchi/flag branch from 1ae9b9d to d4f20e2 Nov 8, 2019
@adrw
adrw approved these changes Nov 8, 2019
@mightyguava mightyguava force-pushed the yunchi/flag branch from d4f20e2 to 35389b8 Nov 8, 2019
@mightyguava mightyguava force-pushed the yunchi/flag branch from 35389b8 to 01b4031 Nov 8, 2019
Copy link
Collaborator

ryanhall07 left a comment

I think this is reasonable, but like you said, it doesn't belong in misk :)

would it really be that much work to add a FeatureFlagVaildator interface that defaults to an AllowAll impl. You would then move your impl to skim

return overrides.getOrElse(MapKey(feature, key)) {
overrides[MapKey(feature)]
}
}

private fun <V> getOrDefault(feature: Feature, key: String, defaultValue: V): V {
FeatureFlagValidation.checkValidKey(feature, key)

This comment has been minimized.

Copy link
@ryanhall07

ryanhall07 Nov 9, 2019

Collaborator

nice

@mightyguava

This comment has been minimized.

Copy link
Collaborator Author

mightyguava commented Nov 11, 2019

would it really be that much work to add a FeatureFlagVaildator interface that defaults to an AllowAll impl. You would then move your impl to skim

Spoke offline. At the moment Franklin can't pull in Misk guice modules for the most part. We'd need to publish a new misk jar and it's not really worth the effort.

@mightyguava mightyguava merged commit a9667eb into master Nov 11, 2019
3 checks passed
3 checks passed
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details
@mightyguava mightyguava deleted the yunchi/flag branch Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.