Skip to content
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.

Sign up for GitHub

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

DDO-3377: Use CreatedAt instead of CommittedAt #398

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+14 −17
Open

DDO-3377: Use CreatedAt instead of CommittedAt #398

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3b5878f
remove committedat
katiewelch Dec 14, 2023
1e3e761
add createdat to git_commits_v3
katiewelch Dec 14, 2023
d0a2d0b
refactor
katiewelch Dec 14, 2023
f4f4ee3
testing
katiewelch Dec 14, 2023
9c9e17d
sql migrations
katiewelch Dec 14, 2023
cf6503c
testing
katiewelch Dec 14, 2023
de8face
new lines :/
katiewelch Dec 14, 2023
bc76dcb
remove commonfields in favor of time.now()
katiewelch Dec 14, 2023
e196279
time.since
katiewelch Dec 14, 2023
34886dc
testing
katiewelch Dec 14, 2023
d721744
testing
katiewelch Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion sherlock-webhook-proxy/pkg/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ func HandleWebhook(w http.ResponseWriter, r *http.Request) {
created, err := sherlockClient.GitCommits.PutAPIGitCommitsV3(&git_commits.PutAPIGitCommitsV3Params{
Context: context.Background(),
GitCommit: &models.SherlockGitCommitV3Upsert{
CommittedAt: payload.Commits[0].Timestamp,
GitBranch: strings.TrimPrefix(payload.Ref, "refs/heads/"),
GitCommit: payload.After,
GitRepo: payload.Repository.Name,
Expand Down
2 changes: 2 additions & 0 deletions sherlock/db/migrations/000069_drop_committed_at_column_from_git_commits.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
alter table git_commits
add column if not exists committed_at timestamp with time zone not null;
2 changes: 2 additions & 0 deletions sherlock/db/migrations/000069_drop_committed_at_column_from_git_commits.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
alter table git_commits
drop column if exists committed_at;
12 changes: 5 additions & 7 deletions sherlock/internal/api/sherlock/git_commits_v3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,15 @@ package sherlock

import (
"github.com/broadinstitute/sherlock/sherlock/internal/models"
"time"
)

type GitCommitV3 struct {
CommonFields
GitRepo string `json:"gitRepo"`
GitCommit string `json:"gitCommit"`
GitBranch string `json:"gitBranch"`
IsMainBranch bool `json:"isMainBranch"`
SecSincePrev *uint `json:"secSincePrev"`
CommittedAt time.Time `json:"committedAt"`
GitRepo string `json:"gitRepo"`
GitCommit string `json:"gitCommit"`
GitBranch string `json:"gitBranch"`
IsMainBranch bool `json:"isMainBranch"`
SecSincePrev *uint `json:"secSincePrev"`
}

//nolint:unused
Expand Down
14 changes: 6 additions & 8 deletions sherlock/internal/api/sherlock/git_commits_v3_upsert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,14 @@ import (
"github.com/broadinstitute/sherlock/sherlock/internal/models"
"github.com/gin-gonic/gin"
"net/http"
"time"
)

type GitCommitV3Upsert struct {
GitRepo string `json:"gitRepo"`
GitCommit string `json:"gitCommit"`
GitBranch string `json:"gitBranch"`
IsMainBranch bool `json:"isMainBranch"`
CommittedAt time.Time `json:"committedAt"`
CommonFields
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

GitRepo string `json:"gitRepo"`
GitCommit string `json:"gitCommit"`
GitBranch string `json:"gitBranch"`
IsMainBranch bool `json:"isMainBranch"`
}

// gitCommitsV3Upsert godoc
Expand Down Expand Up @@ -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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

var result models.GitCommit
if err = db.Where(&models.GitCommit{GitRepo: body.GitRepo, GitBranch: body.GitBranch, GitCommit: body.GitCommit}).
Attrs(&models.GitCommit{
IsMainBranch: body.IsMainBranch,
SecSincePrev: timeSince,
CommittedAt: body.CommittedAt,
}).FirstOrCreate(&result).Error; err != nil {
errors.AbortRequest(ctx, err)
return
Expand Down
2 changes: 0 additions & 2 deletions sherlock/internal/models/git_commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package models

import (
"gorm.io/gorm"
"time"
)

type GitCommit struct {
Expand All @@ -12,5 +11,4 @@ type GitCommit struct {
GitBranch string
IsMainBranch bool
SecSincePrev *uint
CommittedAt time.Time
}
Loading