-
Notifications
You must be signed in to change notification settings - Fork 2
Convert git to https api endpoints #426
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
Conversation
@@ -1,6 +1,6 @@ | |||
// Code generated by protoc-gen-go. DO NOT EDIT. | |||
// versions: | |||
// protoc-gen-go v1.27.1 | |||
// protoc-gen-go v1.26.0 |
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.
Update buf
and re-run cd cmd/clusters-service && make install
to get the current versions of things!
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.
👌 Thank you 🙂
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.
Some minor suggestions but overall this LGTM 🥇
@@ -159,6 +161,8 @@ message DeleteClustersPullRequestRequest { | |||
string commit_message = 7; | |||
// Credentials | |||
Credential credentials = 8; | |||
// The repo api url. | |||
string repository_api_url = 9; |
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.
Just curious, why do we accept it in the request and also send it back in the response?
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.
My mistake, I think we should receive it in request only and if supplied we should ignore creating scm client. Thank you very much for noticing 🙂
cmd/clusters-service/pkg/git/git.go
Outdated
@@ -79,7 +83,12 @@ type CloneRepoToTempDirResponse struct { | |||
// It returns the URL of the pull request. | |||
func (s *GitProviderService) WriteFilesToBranchAndCreatePullRequest(ctx context.Context, | |||
req WriteFilesToBranchAndCreatePullRequestRequest) (*WriteFilesToBranchAndCreatePullRequestResponse, error) { | |||
repo, err := s.GetRepository(ctx, req.GitProvider, req.RepositoryURL) | |||
apiEndpoint, err := getSCMClient(req.GitProvider, req.RepositoryURL) |
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.
How do you think about renaming apiEndpoint
to something else since it's an object of *scm.Client
?
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.
Makes sense to me 👍
cmd/clusters-service/pkg/git/git.go
Outdated
repo, err := s.GetRepository(ctx, req.GitProvider, req.RepositoryURL) | ||
apiEndpoint, err := getSCMClient(req.GitProvider, req.RepositoryURL) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to get api endpoint: %w", err) |
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.
Similarly here, is it the endpoint or the client that we fail to retrieve?
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.
Right! It should be the client 👍
cmd/clusters-service/pkg/git/git.go
Outdated
@@ -293,3 +301,22 @@ func getGitProviderClient(gpi GitProvider) (gitprovider.Client, error) { | |||
} | |||
return client, err | |||
} | |||
|
|||
func getGitProviderUrl(giturl string) (string, error) { |
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.
Wicked! 🚀 , lets add a couple of tests w/ the diff variations
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.
Sure 👍
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.
💯 ! Looks good to me!
Closes #401