-
Notifications
You must be signed in to change notification settings - Fork 499
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
tools/horizon-cmp: Remove unnecessary panics, update temp folder names #1947
Conversation
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.
Couple questions and a suggestion about the temp folder name.
@@ -32,6 +33,7 @@ var removeRegexps = []*regexp.Regexp{ | |||
// regexp.MustCompile(`\s*"transaction_count": [0-9]+,`), | |||
// regexp.MustCompile(`\s*"last_modified_ledger": [0-9]+,`), | |||
// regexp.MustCompile(`\s*"public_key": "G.*",`), | |||
regexp.MustCompile(`,\s*"paging_token": ?""`), |
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.
The PR description doesn't mention changing the regexs that get used. Is this accidental? How does this relate to removing panics and the temp folder change?
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.
I believe it's because we were missing a check. Our paging token diverged from the expected value so we're just future-proofing.
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.
Yes, this is to ignore new paging_token
field added in 0.23.0 that wasn't there in 0.22.1. Commented it for now and I'll think about moving all regexps to a config file.
tools/horizon-cmp/main.go
Outdated
@@ -119,7 +119,7 @@ func run(cmd *cobra.Command) { | |||
if err != nil { | |||
panic(err) | |||
} | |||
outputDir := fmt.Sprintf("%s/horizon-cmp-diff/%d", pwd, time.Now().Unix()) | |||
outputDir := fmt.Sprintf("%s/horizon-cmp-diff/%s", pwd, time.Now().Format(time.RFC3339)) |
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.
This will cause :
to be in folder names which can be problematic. Technically folder names on Unix/Linux systems allow :
but some situations, libraries and applications bork at it, for example a folder including :
can't be included in $PATH
. That specific example shouldn't be relevant here but I think it's still worth avoiding given that we know that developers don't usually think of it as a value that will show up in a valid path.
How about using this format instead with the separators removed, much of the same benefit but a safer filename:
20060102T150405Z0700
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 could use -
separators, would be easier to read than no separators
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 point, changed to 2006-01-02-15-04-05
.
@@ -94,7 +98,7 @@ func NewResponse(domain, path string, stream bool) *Response { | |||
} | |||
|
|||
if string(body) == "" { | |||
panic("Empty body") | |||
response.Body = fmt.Sprintf("Empty body [%d]", rand.Uint64()) |
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.
What problem does the rand in the body help with? Why wouldn't we want just to return an empty body?
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.
I'm guessing it's for debugging - ties the output to a specific instance
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.
It's to make sure that diff is saved when both, base and test, servers somehow return empty responses (otherwise this case would be ignore). In the future, horizon-cmp should retry in such cases.
@@ -32,6 +33,7 @@ var removeRegexps = []*regexp.Regexp{ | |||
// regexp.MustCompile(`\s*"transaction_count": [0-9]+,`), | |||
// regexp.MustCompile(`\s*"last_modified_ledger": [0-9]+,`), | |||
// regexp.MustCompile(`\s*"public_key": "G.*",`), | |||
regexp.MustCompile(`,\s*"paging_token": ?""`), |
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.
I believe it's because we were missing a check. Our paging token diverged from the expected value so we're just future-proofing.
tools/horizon-cmp/main.go
Outdated
@@ -119,7 +119,7 @@ func run(cmd *cobra.Command) { | |||
if err != nil { | |||
panic(err) | |||
} | |||
outputDir := fmt.Sprintf("%s/horizon-cmp-diff/%d", pwd, time.Now().Unix()) | |||
outputDir := fmt.Sprintf("%s/horizon-cmp-diff/%s", pwd, time.Now().Format(time.RFC3339)) |
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 could use -
separators, would be easier to read than no separators
@@ -94,7 +98,7 @@ func NewResponse(domain, path string, stream bool) *Response { | |||
} | |||
|
|||
if string(body) == "" { | |||
panic("Empty body") | |||
response.Body = fmt.Sprintf("Empty body [%d]", rand.Uint64()) |
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.
I'm guessing it's for debugging - ties the output to a specific instance
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.
What
Removes unnecessary
panic
s that stop execution of the app and changes the temp results folder name.Why
Very often developers run
horizon-cmp
over the night. It's important that it continues running when unexpected scenarios occur. This commit also changes the temp results folder name to be more readable (date vs unix timestamp).