-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix: Define new git provider interface #2450
Conversation
553f75f
to
ff03538
Compare
3464fb6
to
c903c25
Compare
Test go app: func main() {
providerFactory := git.NewFactory(logr.Discard())
providerOpts := []git.ProviderWithFn{
git.WithToken("pat", "**a token**"),
}
provider, err := providerFactory.Create(git.AzureGitOpsProviderName, providerOpts...)
if err != nil {
log.Println(err)
return
}
fileContent := "#!/usr/bin/env zsh\n\necho 'niiice'\n"
pr, err := provider.CreatePullRequest(context.Background(), git.PullRequestInput{
RepositoryURL: "https://dev.azure.com/weaveworks/weave-gitops-integration/_git/sunglow-test-repo",
Title: "New PR Title",
Body: "This is a fancy body message",
Head: fmt.Sprintf("new-branch-%d", rand.Int()),
Base: "main",
Commits: []git.Commit{
{
CommitMessage: "Commit message :)",
Files: []git.CommitFile{
{
Path: "/fancy.sh",
Content: &fileContent,
},
},
},
},
})
if err != nil {
log.Println(err)
return
}
log.Println(pr.Link)
} |
fbfb55b
to
8e312ed
Compare
A few wrapper git provider insider. This one is totally independent from go-git-providers. It uses go-git-providers for our existing provider implementations, but uses jenkins-x/go-scm for Azure. Signed-off-by: Balazs Nadasdi <balazs@weave.works>
Co-authored-by: Yiannis <yiannis@weave.works> Signed-off-by: Balazs Nadasdi <balazs@weave.works>
Co-authored-by: Yiannis <yiannis@weave.works> Signed-off-by: Balazs Nadasdi <balazs@weave.works>
Signed-off-by: Balazs Nadasdi <balazs@weave.works>
8e312ed
to
0aec605
Compare
Tested this on BitBucket Server and got the following error
|
Writing down some notes as part of reviewing this PR:
|
That's weird. This error message can only come from The error message should be: -fmt.Errorf("unable to apply options on provider %q: %w", provider, err)
+fmt.Errorf("unable to apply options on provider %q: %w", providerName, err) |
Ok I found the issue 🤦♀️ ggpOpts := []gitprovider.ClientOption{
gitprovider.WithDomain(opts.Hostname),
// ...
if opts.Hostname != "" {
ggpOpts = append(ggpOpts, gitprovider.WithDomain(opts.Hostname))
} |
Signed-off-by: Balazs Nadasdi <balazs@weave.works>
…orks/weave-gitops-enterprise into 2413-git-provider-interface Signed-off-by: Balazs Nadasdi <balazs@weave.works>
…erface Signed-off-by: Balazs Nadasdi <balazs@weave.works>
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.
Thanks again for pushing through this 💯 I've reviewed the code and it looks like we've covered all the weird edge cases that have been added over time. I especially liked how we eliminated some if/else statements. This PR brings more structure to the git provider handling code and gives us a way to simplify this even further through (hopefully) easier refactoring. Well done!
|
||
var err error | ||
|
||
p.client, err = gitlab.NewClient(opts.Token, opts.TokenType, ggpOpts...) |
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.
What is the default value for TokenType?
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.
Empty string.
In most cases we don't use the "type", only here, that's why the WithToken
has two arguments.
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.
its value is coming directly from "request" struct as before.
cmd/clusters-service/pkg/git/git.go
164: providerOpts = append(providerOpts, git.WithToken(gpi.TokenType, gpi.Token))
171: providerOpts = append(providerOpts, git.WithToken(gpi.TokenType, gpi.Token))
175: providerOpts = append(providerOpts, git.WithToken(gpi.TokenType, gpi.Token))
// cmd/clusters-service/pkg/git/git.go
// This is "gpi"
type GitProvider struct {
Token string
TokenType string
Type string
Hostname string
}
|
||
func (g goGitProvider) WriteFilesToBranch(ctx context.Context, log logr.Logger, req writeFilesToBranchRequest, repo gitprovider.OrgRepository) error { | ||
if req.CreateBranch { | ||
if err := g.CreateBranch(ctx, log, repo, req.BaseBranch, req.HeadBranch); err != nil { |
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.
Can we add a couple of comments to indicate which providers (Gitlab?) need this (CreateBranch: true) and why?
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.
It's not provider specific, it's whether we have to create the branch or it already exists. Right not this logic is used only with gitlab, but that's visible from the logic there. From the point of this function, it's up the to the user if they want to create the branch with this call or not (they already have a branch created by a human, or from code).
Co-authored-by: Yiannis <8741709+yiannistri@users.noreply.github.com>
The root cause was a misused pointer reference in `pkg/git/gitprovider.go`. For some reasons `go-git-providers` (GGP) expects a reference to a string as filename. That wouldn't be an issue in most cases, but we tried to convert WGE internal `CommitFile` to GGP's `CommitFile`. In that loop we assigned a reference, but that reference gets updated while we are in the loop: ```go for _, f := range c.Files { adapted = append(adapted, gitprovider.CommitFile{ Path: &f.Path, Content: f.Content, }) } ``` As a result we end up with the same filename for all files, therefore we commit only one file. Other changes: - Updated go-git-providers from 0.14 to 0.15 References: * Previous PR: #2450 * Issue: #2413 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
The root cause was a misused pointer reference in `pkg/git/gitprovider.go`. For some reasons `go-git-providers` (GGP) expects a reference to a string as filename. That wouldn't be an issue in most cases, but we tried to convert WGE internal `CommitFile` to GGP's `CommitFile`. In that loop we assigned a reference, but that reference gets updated while we are in the loop: ```go for _, f := range c.Files { adapted = append(adapted, gitprovider.CommitFile{ Path: &f.Path, Content: f.Content, }) } ``` As a result we end up with the same filename for all files, therefore we commit only one file. Other changes: - Updated go-git-providers from 0.14 to 0.15 References: * Previous PR: #2450 * Issue: #2413 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
The root cause was a misused pointer reference in `pkg/git/gitprovider.go`. For some reasons `go-git-providers` (GGP) expects a reference to a string as filename. That wouldn't be an issue in most cases, but we tried to convert WGE internal `CommitFile` to GGP's `CommitFile`. In that loop we assigned a reference, but that reference gets updated while we are in the loop: ```go for _, f := range c.Files { adapted = append(adapted, gitprovider.CommitFile{ Path: &f.Path, Content: f.Content, }) } ``` As a result we end up with the same filename for all files, therefore we commit only one file. References: * Previous PR: #2450 * Issue: #2413 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
The root cause was a misused pointer reference in `pkg/git/gitprovider.go`. For some reasons `go-git-providers` (GGP) expects a reference to a string as filename. That wouldn't be an issue in most cases, but we tried to convert WGE internal `CommitFile` to GGP's `CommitFile`. In that loop we assigned a reference, but that reference gets updated while we are in the loop: ```go for _, f := range c.Files { adapted = append(adapted, gitprovider.CommitFile{ Path: &f.Path, Content: f.Content, }) } ``` As a result we end up with the same filename for all files, therefore we commit only one file. References: * Previous PR: #2450 * Issue: #2413 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
The root cause was a misused pointer reference in `pkg/git/gitprovider.go`. For some reasons `go-git-providers` (GGP) expects a reference to a string as filename. That wouldn't be an issue in most cases, but we tried to convert WGE internal `CommitFile` to GGP's `CommitFile`. In that loop we assigned a reference, but that reference gets updated while we are in the loop: ```go for _, f := range c.Files { adapted = append(adapted, gitprovider.CommitFile{ Path: &f.Path, Content: f.Content, }) } ``` As a result we end up with the same filename for all files, therefore we commit only one file. References: * Previous PR: #2450 * Issue: #2413 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
The root cause was a misused pointer reference in `pkg/git/gitprovider.go`. For some reasons `go-git-providers` (GGP) expects a reference to a string as filename. That wouldn't be an issue in most cases, but we tried to convert WGE internal `CommitFile` to GGP's `CommitFile`. In that loop we assigned a reference, but that reference gets updated while we are in the loop: ```go for _, f := range c.Files { adapted = append(adapted, gitprovider.CommitFile{ Path: &f.Path, Content: f.Content, }) } ``` As a result we end up with the same filename for all files, therefore we commit only one file. References: * Previous PR: #2450 * Issue: #2413 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
This change-set adds Azure DevOps support to pipeline promotions. Based on discussions (on Slack) we decided to go with a "copy as it is" solution to use the new GitProvider used in WGE (PR[^1] with 2 fixes[^2][^3]). It's a simple copy-paste what we have in WGE, but with a few addition and changes: * [addition] List Pull Requests. If we can't create a PR, we try to find an existing one with Pipelines. That logic does not exist in WGE. * [change] Do not create branch if we have no additional commits. On WGE we create a branch with the CreatePullRequest call (except in on case[^4]), but with pipelines we already have the branch with all required files and changes using pure git operations with clone and push. In this case we don't have additional commits, we only have to create the new branch if `len(input.commits) > 0`. * [addition] `Name()` and `SupportedDomain()`. These new functions were added to make it easier to verify or test given properties hidden behind the implementation of a Provder interface. We may want to port them back to WGE with extra testing. Closes #170 [^1]: weaveworks/weave-gitops-enterprise#2450 [^2]: weaveworks/weave-gitops-enterprise#2585 [^3]: weaveworks/weave-gitops-enterprise#2625 [^4]: https://github.com/weaveworks/weave-gitops-enterprise/blob/main/pkg/git/gitlab.go#L108 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
This change-set adds Azure DevOps support to pipeline promotions. Based on discussions (on Slack) we decided to go with a "copy as it is" solution to use the new GitProvider used in WGE (PR[^1] with 2 fixes[^2][^3]). It's a simple copy-paste what we have in WGE, but with a few addition and changes: * [addition] List Pull Requests. If we can't create a PR, we try to find an existing one with Pipelines. That logic does not exist in WGE. * [change] Do not create branch if we have no additional commits. On WGE we create a branch with the CreatePullRequest call (except in on case[^4]), but with pipelines we already have the branch with all required files and changes using pure git operations with clone and push. In this case we don't have additional commits, we only have to create the new branch if `len(input.commits) > 0`. * [addition] `Name()` and `SupportedDomain()`. These new functions were added to make it easier to verify or test given properties hidden behind the implementation of a Provder interface. We may want to port them back to WGE with extra testing. Additional changes: * Set Go version to 1.20 to match with WGE. Closes #170 [^1]: weaveworks/weave-gitops-enterprise#2450 [^2]: weaveworks/weave-gitops-enterprise#2585 [^3]: weaveworks/weave-gitops-enterprise#2625 [^4]: https://github.com/weaveworks/weave-gitops-enterprise/blob/main/pkg/git/gitlab.go#L108 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
This change-set adds Azure DevOps support to pipeline promotions. Based on discussions (on Slack) we decided to go with a "copy as it is" solution to use the new GitProvider used in WGE (PR[^1] with 2 fixes[^2][^3]). It's a simple copy-paste what we have in WGE, but with a few addition and changes: * [addition] List Pull Requests. If we can't create a PR, we try to find an existing one with Pipelines. That logic does not exist in WGE. * [change] Do not create branch if we have no additional commits. On WGE we create a branch with the CreatePullRequest call (except in on case[^4]), but with pipelines we already have the branch with all required files and changes using pure git operations with clone and push. In this case we don't have additional commits, we only have to create the new branch if `len(input.commits) > 0`. * [addition] `Name()` and `SupportedDomain()`. These new functions were added to make it easier to verify or test given properties hidden behind the implementation of a Provder interface. We may want to port them back to WGE with extra testing. Additional changes: * Set Go version to 1.20 to match with WGE. Closes #170 [^1]: weaveworks/weave-gitops-enterprise#2450 [^2]: weaveworks/weave-gitops-enterprise#2585 [^3]: weaveworks/weave-gitops-enterprise#2625 [^4]: https://github.com/weaveworks/weave-gitops-enterprise/blob/main/pkg/git/gitlab.go#L108 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
This change-set adds Azure DevOps support to pipeline promotions. Based on discussions (on Slack) we decided to go with a "copy as it is" solution to use the new GitProvider used in WGE (PR[^1] with 2 fixes[^2][^3]). It's a simple copy-paste what we have in WGE, but with a few addition and changes: * [addition] List Pull Requests. If we can't create a PR, we try to find an existing one with Pipelines. That logic does not exist in WGE. * [change] Do not create branch if we have no additional commits. On WGE we create a branch with the CreatePullRequest call (except in on case[^4]), but with pipelines we already have the branch with all required files and changes using pure git operations with clone and push. In this case we don't have additional commits, we only have to create the new branch if `len(input.commits) > 0`. * [addition] `Name()` and `SupportedDomain()`. These new functions were added to make it easier to verify or test given properties hidden behind the implementation of a Provder interface. We may want to port them back to WGE with extra testing. Additional changes: * Set Go version to 1.20 to match with WGE. Closes #170 [^1]: weaveworks/weave-gitops-enterprise#2450 [^2]: weaveworks/weave-gitops-enterprise#2585 [^3]: weaveworks/weave-gitops-enterprise#2625 [^4]: https://github.com/weaveworks/weave-gitops-enterprise/blob/main/pkg/git/gitlab.go#L108 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
This change-set adds Azure DevOps support to pipeline promotions. Based on discussions (on Slack) we decided to go with a "copy as it is" solution to use the new GitProvider used in WGE (PR[^1] with 2 fixes[^2][^3]). It's a simple copy-paste what we have in WGE, but with a few addition and changes: * [addition] List Pull Requests. If we can't create a PR, we try to find an existing one with Pipelines. That logic does not exist in WGE. * [change] Do not create branch if we have no additional commits. On WGE we create a branch with the CreatePullRequest call (except in on case[^4]), but with pipelines we already have the branch with all required files and changes using pure git operations with clone and push. In this case we don't have additional commits, we only have to create the new branch if `len(input.commits) > 0`. * [addition] `Name()` and `SupportedDomain()`. These new functions were added to make it easier to verify or test given properties hidden behind the implementation of a Provder interface. We may want to port them back to WGE with extra testing. Additional changes: * Set Go version to 1.20 to match with WGE. Closes #170 [^1]: weaveworks/weave-gitops-enterprise#2450 [^2]: weaveworks/weave-gitops-enterprise#2585 [^3]: weaveworks/weave-gitops-enterprise#2625 [^4]: https://github.com/weaveworks/weave-gitops-enterprise/blob/main/pkg/git/gitlab.go#L108 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
This change-set adds Azure DevOps support to pipeline promotions. Based on discussions (on Slack) we decided to go with a "copy as it is" solution to use the new GitProvider used in WGE (PR[^1] with 2 fixes[^2][^3]). It's a simple copy-paste what we have in WGE, but with a few addition and changes: * [addition] List Pull Requests. If we can't create a PR, we try to find an existing one with Pipelines. That logic does not exist in WGE. * [change] Do not create branch if we have no additional commits. On WGE we create a branch with the CreatePullRequest call (except in on case[^4]), but with pipelines we already have the branch with all required files and changes using pure git operations with clone and push. In this case we don't have additional commits, we only have to create the new branch if `len(input.commits) > 0`. * [addition] `Name()` and `SupportedDomain()`. These new functions were added to make it easier to verify or test given properties hidden behind the implementation of a Provder interface. We may want to port them back to WGE with extra testing. Additional changes: * Set Go version to 1.20 to match with WGE. Closes #170 [^1]: weaveworks/weave-gitops-enterprise#2450 [^2]: weaveworks/weave-gitops-enterprise#2585 [^3]: weaveworks/weave-gitops-enterprise#2625 [^4]: https://github.com/weaveworks/weave-gitops-enterprise/blob/main/pkg/git/gitlab.go#L108 Signed-off-by: Balazs Nadasdi <balazs@weave.works>
Closes #2413
What changed?
A few wrapper git provider insider. This one is totally independent from
go-git-providers
. It uses go-git-providers for our existing providerimplementations, but uses
jenkins-x/go-scm
for Azure.Adds a new provider: Azure GitOps.
Why was this change made?
To make it easier to switch to a different git provider library.
How was this change implemented?
With sweat and tears and blood. A new package (
pkg/git
) creates a new wrapper and factory, the whole system can use then and what library is used behind given provider is not their business. The rest of the codebase just wants to create a PR and they should not care about what provider is used for that.How did you validate the change?
Release notes
Documentation Changes