Skip to content

Conversation

@gwaldo
Copy link
Member

@gwaldo gwaldo commented Jul 2, 2016

ref #567

STATUS: Not yet ready to merge. Reviews and edits welcome

@gwaldo
Copy link
Member Author

gwaldo commented Jul 2, 2016

Alright, @graphite-project/committers I would love a review. Please make it a hearty review, especially given my relative weakness with Python, and knowledge of this particular codebase.

Everything that I found to change was discovered by gratuitous grepping along the lines of grep -rn -i -E 'whitelist|blacklist' ./*, adding obnoxious deprecation comments so I could | grep -v -i deprecat to filter what was left.

@gwaldo
Copy link
Member Author

gwaldo commented Jul 2, 2016

I haven't done any work on graphite-web yet. I don't know if I'll get to work on it tonight, but we'll see. If everything looks good, but we want to wait until both apps' PRs are ready, I'm cool with that.

# PID_DIR = %(STORAGE_DIR)s/
# STORAGE_DIR = $GRAPHITE_STORAGE_DIR
# LOCAL_DATA_DIR = %(STORAGE_DIR)s/whisper/
# USE_METRIC_FILTERS = %(STORAGE_DIR)s/lists/
Copy link
Member

@obfuscurity obfuscurity Jul 7, 2016

Choose a reason for hiding this comment

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

This should be METRIC_FILTERS_DIR, no?

@obfuscurity
Copy link
Member

It looks like this isn't completely backwards compatible yet afa the parser and settings code. We need to be additive with the new stuff but leave the old stuff in place (not in examples/docs/confs, but in the aforementioned parser/settings). Make sense?

@gwaldo
Copy link
Member Author

gwaldo commented Jul 8, 2016

Thank you, @obfuscurity. I haven't had time to look at this since Monday, but I'd had a suspicion that I'd missed one of the backwards-compatibility features. I'll see what I can do this weekend. I need to rebase and proceed.

I've updated the status in the first comment to reflect that this isn't yet ready for merge.

@obfuscurity
Copy link
Member

Closing in favor of #591.

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.

2 participants