Refactoring for better separation of concerns #110
Conversation
The Go standard library net/http.Request struct exposes a special header, which takes effect when issuing http requests. The Host header isn't honored, so I set this Host field from a Host header if present. Closes sourcegraph#79
- separated types into submodule, - defined interfaces into separate go file, - defined error messages in errors.go, - split checks into /check/[name] packages, - split storage into /storage/[name] packages, - split notifiers into /notifier/[name] packages, - test cleanups, move test data into relevant packages No new features have been added, the code base has just been cleaned up to be more readable, and that individual subpackages contain their implementations. With this change, new notifiers and checks can be added, without enroaching into the main checkup package.
The notifier functions are set up to return errors. The slack notifier only issued log calls, but swallowed the error for handling in checkup runs.
I added a Type field and Type functions on checks, notifiers and storage interfaces, to avoid awkward type casting/type detection from json data and go structs
This commit fixes the issue of inconsistent type detection over storage (had 'provider'), notifier (had 'name') and check (had 'type') - not it always uses 'type'. This commit also adds support for multiple checkers via in checkup.json.
titpetric
changed the title
|
@beyang PTAL |
|
|
||
| - providers: the json config field `provider` was renamed to `type` for consistency, | ||
| - notifiers: the json config field `name` was renamed to `type` for consistency, | ||
| - sql: by default the sqlite storage engine is disabled (needs build with `-tags sql` to enable), |
There was a problem hiding this comment.
If there's any way to call this out more aggressively, that would be great! Perhaps a CHANGELOG? My crons stopped running overnight due to (1) me using r.j3ss.co/checkup:latest, which tracks HEAD and (2) these changes. Specifically, the provider -> type change for storage caught me. For anyone relying on HEAD for their production checks, this is likely to bite them.
There was a problem hiding this comment.
I'm very sorry about that. It was a risk I was aware of when I was cleaning up the code for this, and I just hoped people aren't blindly doing nightly/HEAD builds, and tried to make it as obvious as possible that there was a breaking change introduced by prominently updating the README at the very top.
Aside from adding a CHANGELOG file,
- we can decide on a branching model (master|develop, v0, v1...), AND
- we can remove tags as they are unused/out of date (new releases needed #113 Project status? #86 slack: unknown Notifier type #84 ...) OR
- we can tag a 0.3.0 before breaking changes and 0.4.0 after
And finally, we could automate the release process in line with what we choose above (#96). There are also a number of issues that could be solved by having a rolling release model:
- new releases needed #113
- Project status? #86
- slack: unknown Notifier type #84 (this is an issue that just needs a new tagged build)
- Is the project abandoned ? #46
The current releases are woefully out of date. I'm voting to remove them, keep the development on master, branch to v0 before the breaking changes (f4747239), and branch into v1 later when we feel the release is pretty stable and non-breaking.
@beyang thoughts?
There was a problem hiding this comment.
@parkr sorry this bit you. We don't have a good sense of who's using this at the moment, so yes, we should probably adopt a versioning system as @titpetric proposes. @titpetric the model you propose sounds good to me. I'd propose this model:
masteris the development branch- We adopt semver. I've pushed tags
v1.0.0(the last commit before @titpetric's recent changes) andv2.0.0(the latestmastercommit). I've also pushed a1.0branch in case we want to cut any new patches off the older version
@titpetric feel free to push up a CHANGELOG file as well. Also not we did publish release notes for early versions of Checkup: https://github.com/sourcegraph/checkup/releases.
I refactored checkup in various ways:
Some changes are breaking changes due to existing configs (renamed type/name/provider -> type for consistency). There was no need to have three different keys to describe the configuration.
The documentation is updated. It shouldn't affect many people, as the breaking change would only happen if people are building the
masterbranch from source. For newcomers the README.md is up to date.