-
Notifications
You must be signed in to change notification settings - Fork 501
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
services/horizon: Remove all references to Redis #2409
Conversation
fd5c0e0
to
97b3aa3
Compare
It wasn't used, it was only adding complexity and causing test failures.
97b3aa3
to
0adecb2
Compare
We should deprecate them first, to give room to remove them (in case someone is using them)
ConfigKey: &config.RedisURL, | ||
OptType: types.String, | ||
Usage: "redis to connect with, for rate limiting", | ||
&support.ConfigOption{ // To be removed in horizon 2.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to add a specially formatted comment following the pattern outlined in issue #833 so that CI picks up on them and helps us remove them when the time comes. @stellar/horizon-committers I can't find any details in our CONTRIBUTING.md or DEVELOPING.md documents that outline how to do this. Have we abandoned that practice?
We still have the CI check for this (below), so I think we're still wanting to do it and probably need some better docs to encourage us to use it. I know I've probably not done this properly recently too 😬.
Lines 41 to 52 in bab6abc
# check_deprecations ensures a release is actually removing deprecated fields | |
# that were supposed to be discontinued in said release. | |
check_deprecations: | |
steps: | |
- run: | |
name: Run deprecation tests when on a tagged commit | |
command: | | |
if [ "$CIRCLE_TAG" != "" ]; then | |
# Negate the result so process exits with 1 if anything found | |
echo "Searching for \"action needed\" tags..." | |
! egrep -irn -A 1 --include=*.go "Action.+needed.+in.+release:.+$CIRCLE_TAG" ./ | |
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. The approach isn't abandoned. It's documented in our internal devel guide.
Mark deprecated public API fields using deprecation tag:
// Deprecated - remove in: horizon-v[version]
We should do a pass over this and update DEVELOPING.md with things relevant to all contributors.
@2opremio can you update the deprecation comments to match the syntax quoted above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2412 for the doc updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing
I notice that this change was made only for the next Horizon release branch. Is there a reason this isn't on master? I may be making changes to the CircleCI config soon and we can avoid merge conflicts later when Horizon releases if this change is also on master. |
Sure, I will create a PR to merge it back to master. |
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Fixes #2395
What
Remove all Redis references.
Why
It wasn't used, it was only adding complexity and causing test failures.
Known limitations
N/A