feat(hydrator): add commit-server component #19537
Conversation
|
@CodiumAI-Agent /review |
PR Reviewer Guide 🔍
|
crenshaw-dev
force-pushed
the
source-hydrator-types
branch
from
79829ec
to
2da30b8
Compare
crenshaw-dev
force-pushed
the
commit-server
branch
2 times, most recently
from
17d228e
to
978b253
Compare
crenshaw-dev
force-pushed
the
source-hydrator-types
branch
from
2da30b8
to
5a16330
Compare
crenshaw-dev
force-pushed
the
commit-server
branch
from
978b253
to
a2ebf59
Compare
crenshaw-dev
force-pushed
the
source-hydrator-types
branch
from
5a16330
to
84f44f8
Compare
crenshaw-dev
force-pushed
the
commit-server
branch
2 times, most recently
from
2d34f7f
to
5cc7db9
Compare
crenshaw-dev
force-pushed
the
source-hydrator-types
branch
from
84f44f8
to
e97f0ff
Compare
Co-authored-by: Alexandre Gaudreault <[email protected]> Co-authored-by: Omer Azmon <[email protected]> Co-authored-by: daengdaengLee <[email protected]> Co-authored-by: Juwon Hwang (Kevin) <[email protected]> Co-authored-by: thisishwan2 <[email protected]> Co-authored-by: mirageoasis <[email protected]> Co-authored-by: Robin Lieb <[email protected]> Co-authored-by: miiiinju1 <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> go mod tidy Signed-off-by: Michael Crenshaw <[email protected]> one test file for both implementations Signed-off-by: Michael Crenshaw <[email protected]> simplify Signed-off-by: Michael Crenshaw <[email protected]> fix test for linux Signed-off-by: Michael Crenshaw <[email protected]> fix git client mock Signed-off-by: Michael Crenshaw <[email protected]> fix git client mock Signed-off-by: Michael Crenshaw <[email protected]> address comments Signed-off-by: Michael Crenshaw <[email protected]> unit tests Signed-off-by: Michael Crenshaw <[email protected]> lint Signed-off-by: Michael Crenshaw <[email protected]> fix image, fix health checks, fix merge issue Signed-off-by: Michael Crenshaw <[email protected]> fix lint issues Signed-off-by: Michael Crenshaw <[email protected]>
crenshaw-dev
force-pushed
the
commit-server
branch
from
5cc7db9
to
af7600a
Compare
| @@ -43,6 +43,7 @@ require ( | |||
| github.com/golang/protobuf v1.5.4 | |||
| github.com/google/btree v1.1.3 | |||
| github.com/google/go-cmp v0.6.0 | |||
| github.com/google/go-github/v62 v62.0.0 | |||
There was a problem hiding this comment.
v63 already imported below
| github.com/google/go-github/v62 v62.0.0 | |
| github.com/google/go-github/v62 v62.0.0 |
| - mountPath: /helm-working-dir | ||
| name: helm-working-dir | ||
| - mountPath: /home/argocd/cmp-server/plugins | ||
| name: plugins |
There was a problem hiding this comment.
This seems to be stuff copied from the repo-server that should be removed if commit-server is not doing helm/cmp operations
| // CommitServerSocketPath is the path to the socket used by the commit server to communicate with the askpass server | ||
| CommitServerSocketPath = "/tmp/commit-server-ask-pass.sock" |
There was a problem hiding this comment.
Move file to util/askpass/common.go instead. Current location is reposerver/askpass/common.go and it now contains information related to commit-server
| } | ||
| return nil | ||
| return "", nil |
There was a problem hiding this comment.
Why adding the "out" parameter if the function always returns "" on success?
| func (m *nativeGitClient) CheckoutOrNew(branch, base string, submoduleEnabled bool) (string, error) { | ||
| out, err := m.Checkout(branch, submoduleEnabled) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "did not match any file(s) known to git") { |
There was a problem hiding this comment.
Can we do a call to validate if the branch exist or check the error code instead of the error message? I wouldn't trust the consistency of git client error string for such a critical functionality.
| @@ -245,6 +266,13 @@ func NewSSHCreds(sshPrivateKey string, caPath string, insecureIgnoreHostKey bool | |||
| return SSHCreds{sshPrivateKey, caPath, insecureIgnoreHostKey, store, proxy, noProxy} | |||
| } | |||
|
|
|||
| // GetUserInfo returns empty strings for user info. | |||
| // TODO: Implement this method to return the username and email address for the credentials, if they're available. | |||
There was a problem hiding this comment.
Create issues if this is still TODO and it could be implemented in another PR
| s.metricsServer.IncCommitRequest(repoURL, metrics.CommitResponseTypeFailure) | ||
| s.metricsServer.ObserveCommitRequestDuration(repoURL, metrics.CommitResponseTypeFailure, time.Since(startTime)) |
There was a problem hiding this comment.
you can wrap that in a defer function that checks if the return error is not nil. This way you are sure to execute in in the future
func example() (err error) {
startTime := time.Now()
defer func() {
if err != nil {
s.metricsServer.IncCommitRequest(repoURL, metrics.CommitResponseTypeFailure)
}
s.metricsServer.IncCommitRequest(repoURL, metrics.CommitResponseTypeSuccess)
s.metricsServer.ObserveCommitRequestDuration(repoURL, metrics.CommitResponseTypeFailure, time.Since(startTime))
}
return nil
}
I'm breaking the hydrator into separate PRs. This is party to make them easier to review, partly because the commit history on the original PR is pretty messy at this point.
This PR is just for the new commit-server component. In this code, the commit server isn't invoked by anything besides tests, and the k8s manifests are not included in the rendered install manifests.
I've added everyone who helped as a co-author on this PR.