fix: Define new git provider interface #2450

yitsushi · 2023-02-23T11:36:11Z

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 provider
implementations, 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?

Existing tests passed
Manual tested GitHub and BitBucket flow.
Manual test on Azure through the UI is blocked on oauth stuff, but using the provider in a test go app I could create a PR.

Release notes

Documentation Changes

yitsushi · 2023-03-14T15:59:45Z

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)
}

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>

Signed-off-by: Balazs Nadasdi <balazs@weave.works>

yiannistri · 2023-03-20T12:04:09Z

Tested this on BitBucket Server and got the following error

failed to access repo https://stash.stashtestserver.link:7990/scm/~yiannis-user/bitbucket.git: unable to get a git provider client for "bitbucket-server": unable to apply options on provider &{{%!q(*zapr.zapLogger=&{0xc000876700 error false true}) '\x00'} %!q(*stash.ProviderClient=<nil>)}: option Domain already configured: invalid options given to NewClient()

yiannistri · 2023-03-20T16:44:56Z

Writing down some notes as part of reviewing this PR:
In
cmd/clusters-service/pkg/server/clusters.go,
cmd/clusters-service/pkg/server/automations.go,
cmd/clusters-service/pkg/server/templates.go and
cmd/clusters-service/pkg/server/helpers.go

github.com/fluxcd/go-git-providers/gitprovider.CommitFile has been replaced by github.com/weaveworks/weave-gitops-enterprise/pkg/git.CommitFile

yitsushi · 2023-03-21T11:25:57Z

Tested this on BitBucket Server and got the following error

failed to access repo https://stash.stashtestserver.link:7990/scm/~yiannis-user/bitbucket.git: unable to get a git provider client for "bitbucket-server": unable to apply options on provider &{{%!q(*zapr.zapLogger=&{0xc000876700 error false true}) '\x00'} %!q(*stash.ProviderClient=)}: option Domain already configured: invalid options given to NewClient()

That's weird. This error message can only come from pkg/git/factory.go. Specifically from func (f *ProviderFactory) Create, and that creates a new go-git-provider stash.NewStashClient client.

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)

yitsushi · 2023-03-21T11:28:21Z

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>

yiannistri

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!

cmd/clusters-service/pkg/server/clusters.go

pkg/git/azure.go

yiannistri · 2023-03-21T14:02:20Z

pkg/git/gitlab.go

+
+	var err error
+
+	p.client, err = gitlab.NewClient(opts.Token, opts.TokenType, ggpOpts...)


What is the default value for TokenType?

Empty string.

In most cases we don't use the "type", only here, that's why the WithToken has two arguments.

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 }

pkg/git/gitprovider.go

yiannistri · 2023-03-21T14:34:31Z

pkg/git/gitprovider.go

+
+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 {


Can we add a couple of comments to indicate which providers (Gitlab?) need this (CreateBranch: true) and why?

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. 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>

yitsushi force-pushed the 2413-git-provider-interface branch 3 times, most recently from 553f75f to ff03538 Compare March 13, 2023 11:15

yitsushi added the enhancement New feature or request label Mar 13, 2023

yitsushi force-pushed the 2413-git-provider-interface branch 2 times, most recently from 3464fb6 to c903c25 Compare March 14, 2023 14:38

yitsushi marked this pull request as ready for review March 14, 2023 16:02

yiannistri requested review from luizbafilho and yiannistri March 15, 2023 13:02

yiannistri force-pushed the 2413-git-provider-interface branch from fbfb55b to 8e312ed Compare March 16, 2023 14:41

yiannistri and others added 4 commits March 16, 2023 18:17

fix: Define new git provider interface

240f25d

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>

fix: Use WGE code instead of OSS

73ad7f7

Co-authored-by: Yiannis <yiannis@weave.works> Signed-off-by: Balazs Nadasdi <balazs@weave.works>

fix: Update provider name for Azure DevOps

defcdc8

Co-authored-by: Yiannis <yiannis@weave.works> Signed-off-by: Balazs Nadasdi <balazs@weave.works>

update getSopsKustomization

0aec605

Signed-off-by: Balazs Nadasdi <balazs@weave.works>

yitsushi force-pushed the 2413-git-provider-interface branch from 8e312ed to 0aec605 Compare March 16, 2023 17:21

Merge branch 'main' into 2413-git-provider-interface

590b89c

yitsushi added 3 commits March 21, 2023 12:29

fix bitbucket server double host definition and error message

424176c

Signed-off-by: Balazs Nadasdi <balazs@weave.works>

Merge branch '2413-git-provider-interface' of ssh://github.com/weavew…

64cc1fa

…orks/weave-gitops-enterprise into 2413-git-provider-interface Signed-off-by: Balazs Nadasdi <balazs@weave.works>

Merge remote-tracking branch 'origin/main' into 2413-git-provider-int…

05618df

…erface Signed-off-by: Balazs Nadasdi <balazs@weave.works>

yiannistri approved these changes Mar 21, 2023

View reviewed changes

yiannistri reviewed Mar 21, 2023

View reviewed changes

yitsushi and others added 2 commits March 21, 2023 16:40

Apply suggestions from code review

26d011d

Co-authored-by: Yiannis <8741709+yiannistri@users.noreply.github.com>

Merge branch 'main' into 2413-git-provider-interface

8111709

yiannistri mentioned this pull request Mar 21, 2023

[UI] Authenticate with github/gitlab button might not be re-enabling properly #2577

Closed

Merge branch 'main' into 2413-git-provider-interface

9326f85

yitsushi merged commit d269690 into main Mar 22, 2023

yitsushi deleted the 2413-git-provider-interface branch March 22, 2023 11:15

yitsushi mentioned this pull request Mar 22, 2023

Implement interface for Azure DevOps #2414

Closed

yitsushi mentioned this pull request Mar 22, 2023

fix: the new git provider didn't commit all required files #2585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Define new git provider interface #2450

fix: Define new git provider interface #2450

yitsushi commented Feb 23, 2023 •

edited

Loading

yitsushi commented Mar 14, 2023 •

edited

Loading

yiannistri commented Mar 20, 2023

yiannistri commented Mar 20, 2023 •

edited

Loading

yitsushi commented Mar 21, 2023 •

edited

Loading

yitsushi commented Mar 21, 2023

yiannistri left a comment •

edited

Loading

yiannistri Mar 21, 2023

yitsushi Mar 21, 2023

yitsushi Mar 21, 2023 •

edited

Loading

yiannistri Mar 21, 2023 •

edited

Loading

yitsushi Mar 21, 2023


		var err error

		p.client, err = gitlab.NewClient(opts.Token, opts.TokenType, ggpOpts...)

fix: Define new git provider interface #2450

fix: Define new git provider interface #2450

Conversation

yitsushi commented Feb 23, 2023 • edited Loading

yitsushi commented Mar 14, 2023 • edited Loading

yiannistri commented Mar 20, 2023

yiannistri commented Mar 20, 2023 • edited Loading

yitsushi commented Mar 21, 2023 • edited Loading

yitsushi commented Mar 21, 2023

yiannistri left a comment • edited Loading

Choose a reason for hiding this comment

yiannistri Mar 21, 2023

Choose a reason for hiding this comment

yitsushi Mar 21, 2023

Choose a reason for hiding this comment

yitsushi Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

yiannistri Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

yitsushi Mar 21, 2023

Choose a reason for hiding this comment

yitsushi commented Feb 23, 2023 •

edited

Loading

yitsushi commented Mar 14, 2023 •

edited

Loading

yiannistri commented Mar 20, 2023 •

edited

Loading

yitsushi commented Mar 21, 2023 •

edited

Loading

yiannistri left a comment •

edited

Loading

yitsushi Mar 21, 2023 •

edited

Loading

yiannistri Mar 21, 2023 •

edited

Loading