-
Notifications
You must be signed in to change notification settings - Fork 250
Refactoring for better separation of concerns #110
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
80be22d
httpchecker: pass host header to http request
ba57107
cleanup: A larger rework/cleanup
5ed18db
notifier slack: prevent swallowing errors
ba94c0e
chore: fixing formatting issues with gofmt
8ad096d
Makefile: revert to go test, add go fmt ./...
603b070
checkup: update checks, notifiers and storage with types
687e65d
feature: add mail notifier and docs
31bd327
Makefile: tune test verbosity to non-verbose
19203b6
checkup: Fix type names in configs for consistency
fdb4608
sql storage: fix compile error, add build-sql target into makefile
f5a941e
documentation: update to reflect recent changes and additions
b17f4c1
chore: added go mod tidy as a build step to clean up go.mod/sum files
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,22 @@ | ||
| .PHONY: all build test docker | ||
| .PHONY: all build build-sql test docker | ||
|
|
||
| all: build test | ||
|
|
||
| DOCKER_IMAGE := checkup | ||
|
|
||
| build: | ||
| go fmt ./... | ||
| go mod tidy | ||
| mkdir -p builds/ | ||
| go build -o builds/ ./cmd/... | ||
|
|
||
| build-sql: | ||
| go fmt ./... | ||
| go mod tidy | ||
| go build -o builds/ -tags sql ./cmd/... | ||
|
|
||
| test: | ||
| go test -race -count=1 -v ./... | ||
| go test -race -count=1 ./... | ||
|
|
||
| docker: | ||
| docker build --no-cache . -t $(DOCKER_IMAGE) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package checkup | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
|
|
||
| "github.com/sourcegraph/checkup/check/dns" | ||
| "github.com/sourcegraph/checkup/check/exec" | ||
| "github.com/sourcegraph/checkup/check/http" | ||
| "github.com/sourcegraph/checkup/check/tcp" | ||
| "github.com/sourcegraph/checkup/check/tls" | ||
| ) | ||
|
|
||
| func checkerDecode(typeName string, config json.RawMessage) (Checker, error) { | ||
| switch typeName { | ||
| case dns.Type: | ||
| return dns.New(config) | ||
| case exec.Type: | ||
| return exec.New(config) | ||
| case http.Type: | ||
| return http.New(config) | ||
| case tcp.Type: | ||
| return tcp.New(config) | ||
| case tls.Type: | ||
| return tls.New(config) | ||
| default: | ||
| return nil, fmt.Errorf(errUnknownCheckerType, typeName) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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->typechange forstoragecaught me. For anyone relying on HEAD for their production checks, this is likely to bite them.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 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,
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 branchv1.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
CHANGELOGfile as well. Also not we did publish release notes for early versions of Checkup: https://github.com/sourcegraph/checkup/releases.