DDO-3377: Use CreatedAt instead of CommittedAt #398
Conversation
What's Changed
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #398 +/- ##
==========================================
+ Coverage 68.56% 68.61% +0.05%
==========================================
Files 165 165
Lines 10711 10703 -8
==========================================
Hits 7344 7344
+ Misses 2864 2856 -8
Partials 503 503
|
There was a problem hiding this comment.
thought: This PR does two things that normally would not be okay, but I think it is okay here:
One, the cloud function and the backend are being changed by the same PR, but the cloud function will get immediately auto-deployed (in practice, before we would've deployed the new Sherlock). This means Sherlock before this PR would receive some incoming git-commit requests lacking the CommittedAt field, which'll make it unhappy.
Two, this PR drops a column while it is still in use (both the old Sherlock and the new Sherlock will coexist briefly during an update). Old Sherlock would get very confused and unhappy if it tries to read or write the CommittedAt column if new Sherlock has already migrated and dropped it.
But... this table and everything here is completely isolated and we're going to throw out the data anyway, we're just moving fast to collect data. I don't think we should worry about those two notes I just made; who cares if we get an ugly row or two.
| GitBranch string `json:"gitBranch"` | ||
| IsMainBranch bool `json:"isMainBranch"` | ||
| CommittedAt time.Time `json:"committedAt"` | ||
| CommonFields |
There was a problem hiding this comment.
| CommonFields |
suggestion: I don't think we actually want to allow upserting the CommonFields, those are typically ready only (the underlying gorm.Model is handled by Gorm, so I actually don't know what would happen if you tried to manually set these)
| @@ -54,15 +53,14 @@ func gitCommitsV3Upsert(ctx *gin.Context) { | |||
| var timeSince *uint | |||
|
|
|||
| if len(previous) > 0 { | |||
| timeSince = utils.PointerTo(uint(body.CommittedAt.Sub(previous[0].CommittedAt).Seconds())) | |||
| timeSince = utils.PointerTo(uint(body.CreatedAt.Sub(previous[0].CreatedAt).Seconds())) | |||
There was a problem hiding this comment.
suggestion: Based on my comment above, the body won't have a .CreatedAt. Instead I think we should just use time.Now() (not leaving it as a GitHub suggestion because you'd need to import "time" too, which GoLand will do if you write it yourself)
|
We are trying to recording the time between git pushes. We originally used the github webhooks push event - commit timestamp to record the time of the push (there is no webhook for the time when the push itself occurred).
If the developer pushes more than one commit at a time, it will record the time of the first commit. We noticed that for at least one developer, they had pushed more than one commit at once, but the commits were more than a month apart.
In order to not skew our data, we determined it would be better to use the CreatedAt field to calculate the time between pushes instead.
This change involves a database migration to remove the committed_at column from the git_commits table.
Testing for Database Migration
I tested this locally by
migrate -path sherlock/db/migrations -database 'postgres://sherlock:password@localhost:5432/sherlock?sslmode=disable' goto 68and
migrate -path sherlock/db/migrations -database 'postgres://sherlock:password@localhost:5432/sherlock?sslmode=disable' goto 69to test down and up, respectively.
Testing for Webhook Handler
I tested following the instructions in the README.
For the 3rd command, I instead used
gh webhook forward --repo=broadinstitute/sherlock --events=push --url=http://localhost:8090/webhook --secret=foobarso that it would run for apushevent instead ofworkflow_run.make local-upcd sherlock-webhook-proxy && FUNCTION_TARGET="HandleWebhook" IAP_TOKEN=$(thelma auth iap --echo) SHERLOCK_URL=http://localhost:8080 GITHUB_WEBHOOK_SECRET=foobar ALLOWED_GITHUB_ORGS=broadinstitute go run cmd/main.go