-
Notifications
You must be signed in to change notification settings - Fork 128
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
service config version [T1031] #322
service config version [T1031] #322
Conversation
…t-version-as-metric
…ice-config-version
store diffs as artifact
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.
LGTM in general, but I have some (dumb) questions.
@@ -43,17 +43,61 @@ const ( | |||
Less ComparisonStatus = iota - 1 // -1 | |||
Equal // 0 | |||
Greater // 1 | |||
InvalidFlags |
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 seems out-of-place here. Isn't this an error which should be reported accordingly?
I'd expect the comparison flags to be inlined at every call site. Therefore using an incorrect set of flags is definitely a programmer error and we can panic if the flags are invalid.
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 a bad pattern to decide what should do unknown program. To handle panics in golang programs programmer should to wrap this function which will defer function with `recover()`` call if he wants to handle an error in place where it has occurred
I added this flag for cases when wasn't used any flag (pass 0) or incorrect value. Function should compare two values and return any less/equal/greater. But in other cases with undefined (oh, maybe use undefined
?) value used some value that will show it and allow to handle it.
Anyway programmer will test his code with comparisons and catch his mistakes. He can write if v1.CompareOnly(0, v2) <operator> <value
where operator may be == or != (>, >=, <=, < is unexpected and not supported operators as any of other implementations of compare
in golang/c/c++). If he expect that result == Equal
or != Equal
, he will take it anyway if pass incorrect flag : )
or we need +1 opinion how will be better in this case )
@@ -1,5 +1,31 @@ | |||
version: 2 | |||
jobs: | |||
general_validation: |
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 is very good move!!
configs/acra-encryptor.example.yaml
Outdated
@@ -1,4 +1,3 @@ | |||
schemas: |
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.
why this line is removed? is it a bug in yaml generator and on first line there should be a version num?
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.
@Lagovas I'm really concerned about this one
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 a mistake. nice catch. maybe removed locally and added via git add
by mistake
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.
ok, thank you!
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 is massive! 😎
configs/
are up to date. it check by generating fresh configs to temporary folder and comparing it toconfigs/*
filesgeneral_validation
where run.circleci/check_[gofmt|golint|gotest]