Rollouts gitlab support #3853

droot · 2023-03-02T21:47:44Z

This PR adds support for GitLab as package source.

Note: Testing individual components is turning out to be tricky. I have added tests for package discovery for gitlab and github that avoids running controller but the test setup is not ideal. I would like to use gitlab community edition for spinning up git repository with some test data, but I want to tackle that as separate task.

droot · 2023-03-03T00:44:26Z

@ChristopherFry @natasha41575 Would be great if you both of you can take a look at it.

droot · 2023-03-03T00:47:12Z

@ChristopherFry @natasha41575 The tests are not ideal in current form But they are helpful if one is hacking on packagediscovery itself. I plan to address them by removing the dependency on gitlab with #3855.

natasha41575

Minor comments, but I have a bigger question.

Porch also has some code to discover packages from Github. Is there any overlap here? I'm wondering if we will eventually want to combine them into a git-package-discovery library.

natasha41575 · 2023-03-03T00:59:06Z

rollouts/api/v1alpha1/rollout_types.go

+type GitLabSource struct {
+	// SecretReference is the reference to a kubernetes secret
+	// that contains GitLab access token
+	SecretRef SecretReference `json:"secretRef,omitempty"`


The GithubSource has the SecretRef in the Selector. Is there a reason that you decided to do it differently for GitLabSource?

Good question. The reasoning for separating SecretRef from Selector is that SecretRef is relevant to How to connect to the Package Source and Selector is relevant to How to select packages from a source. So mixing the two felt odd so decided to do it the right way for GitLab.

I intend to do the same for GitHub as well but in a separate PR.

natasha41575 · 2023-03-03T19:31:34Z

rollouts/pkg/packagediscovery/github_test.go

+		SourceType: gitopsv1alpha1.GitHub,
+		GitHub: gitopsv1alpha1.GitHubSource{
+			Selector: gitopsv1alpha1.GitHubSelector{
+				Org:       "droot",


Should we create these test packages under https://github.com/GoogleContainerTools/kpt/tree/main/package-examples (or some other place under GoogleContainerTools)? I think using your username is ok short-term, but long term it would probably be best to have it somewhere more official and not tied to someone's individual account

So two things:

Absolutely agree on keeping it in an official place. Mine is just a stop gap because of lack of a place.

We can't use ...kpt/tree/main/package-examples for this because it complicates adding/deleting tests. One has to make changes in the test packages and commit it, then update the tests because tests will use whatever is available on main. kpt-samples could be another place but that is used for reference packages and using it for tests can make it really noisy.

But need to find a solution.

Perhaps a kpt-test repo?

natasha41575 · 2023-03-03T19:37:41Z

rollouts/pkg/packagediscovery/packagediscovery.go

 	if isSelectorField(selector.Directory) {
-		tree, _, err := gitHubClient.Git.GetTree(ctx, selector.Org, repoName, selector.Revision, true)
+		tree, _, err := gitHubClient.Git.GetTree(ctx, selector.Org, *repo.Name, repo.GetDefaultBranch(), true)


Why are we using repo.GetDefaultBranch here instead of branch?

Good catch, fixed it.

natasha41575 · 2023-03-03T19:39:18Z

rollouts/pkg/packagediscovery/packagediscovery.go

+	// initialize a gitlab client
+	secretName := gitlabSource.SecretRef.Name
+	if secretName == "" {
+		return nil, fmt.Errorf("GitLab secret reference is missing from the config")


super nit: in this error message, we are camel-casing GitLab, but in later error messages, we leave it all lowercase (gitlab). I think we should pick one for consistency

Good catch, using lower-case everywhere for now.

natasha41575 · 2023-03-03T19:40:04Z

rollouts/pkg/packagediscovery/packagediscovery.go

+	}
+
+	accessToken := string(repositorySecret.Data["token"])
+	// TODO(droot): BaseURL should also be configurable through the API


question: what other baseURLs are possible with gitlab?

Two use-cases that I have in my mind:

Enterprises can self-host gitlab instances and in that case, capability to point to self-hosted gitlab will be needed

Eventually, we will use gitlab docker instance for our e2e tests where being able to point to our instance will be needed.

droot

Thanks for the review @natasha41575

droot · 2023-03-06T22:27:22Z

rollouts/api/v1alpha1/rollout_types.go

+type GitLabSource struct {
+	// SecretReference is the reference to a kubernetes secret
+	// that contains GitLab access token
+	SecretRef SecretReference `json:"secretRef,omitempty"`


Good question. The reasoning for separating SecretRef from Selector is that SecretRef is relevant to How to connect to the Package Source and Selector is relevant to How to select packages from a source. So mixing the two felt odd so decided to do it the right way for GitLab.

I intend to do the same for GitHub as well but in a separate PR.

droot · 2023-03-06T22:31:53Z

rollouts/pkg/packagediscovery/github_test.go

+		SourceType: gitopsv1alpha1.GitHub,
+		GitHub: gitopsv1alpha1.GitHubSource{
+			Selector: gitopsv1alpha1.GitHubSelector{
+				Org:       "droot",


So two things:

Absolutely agree on keeping it in an official place. Mine is just a stop gap because of lack of a place.

We can't use ...kpt/tree/main/package-examples for this because it complicates adding/deleting tests. One has to make changes in the test packages and commit it, then update the tests because tests will use whatever is available on main. kpt-samples could be another place but that is used for reference packages and using it for tests can make it really noisy.

But need to find a solution.

droot · 2023-03-06T22:47:29Z

I'm wondering if we will eventually want to combine them into a git-package-discovery library.

Eventually, yes once we understand the usage better.

ChristopherFry

Looks good!

Not needed now, but In a future change I would definitely suggest splitting the GitHub and GitLab logic into separate files. The Package Discovery file is getting long.

ChristopherFry · 2023-03-06T23:11:54Z

rollouts/Makefile

@@ -174,3 +174,14 @@ $(CONTROLLER_GEN): $(LOCALBIN)
 envtest: $(ENVTEST) ## Download envtest-setup locally if necessary.
 $(ENVTEST): $(LOCALBIN)
 	test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
+


nit: there's an extra newline here

ChristopherFry · 2023-03-06T23:23:42Z

rollouts/controllers/rollout_controller.go

@@ -453,8 +458,9 @@ func (r *RolloutReconciler) computeTargets(ctx context.Context,

 func pkgNeedsUpdate(rollout *gitopsv1alpha1.Rollout, rrs gitopsv1alpha1.RemoteRootSync, pkg *packagediscovery.DiscoveredPackage) (*gitopsv1alpha1.RemoteRootSync, bool) {
 	// TODO: We need to check other things here besides git.Revision and metadata
+	metadata := getSpecMetadata(rollout)


I would consider creating a pull request with just this change since this change is technically unrelated to the rest of the changes in the pull request.

Thanks for pointing it out. This will go away once @natasha41575 PR (that has the fix) is merged.

droot · 2023-03-07T00:46:24Z

Not needed now, but In a future change I would definitely suggest splitting the GitHub and GitLab logic into separate files. The Package Discovery file is getting long.

@ChristopherFry Went ahead and did it in the last commit. At some point, I want to introduce an interface for package selection and this sets up well for that change in the future :)

natasha41575 · 2023-03-07T17:47:03Z

rollouts/controllers/rollout_controller.go

@@ -463,6 +469,25 @@ func pkgNeedsUpdate(rollout *gitopsv1alpha1.Rollout, rrs gitopsv1alpha1.RemoteRo
 	return nil, false
 }

+func getSpecMetadata(rollout *gitopsv1alpha1.Rollout) *gitopsv1alpha1.Metadata {


I think this PR needs a rebase on main. My PR with this fix merged, but I believe I moved this function to the end of the file in my PR and we should try to avoid something funny happening when this PR is merged.

I removed my changes to facilitate clean merge. I had to make small change to make the code aware of syncTemplate being nil.

droot marked this pull request as ready for review March 3, 2023 00:43

droot requested a review from a team as a code owner March 3, 2023 00:43

droot requested review from natasha41575 and ChristopherFry March 3, 2023 00:43

droot mentioned this pull request Mar 3, 2023

Package discovery module for gitlab #3774

Closed

natasha41575 reviewed Mar 3, 2023

View reviewed changes

droot commented Mar 6, 2023

View reviewed changes

droot force-pushed the rollouts-gitlab-support branch from 10799ca to d173d1c Compare March 6, 2023 22:45

ChristopherFry approved these changes Mar 6, 2023

View reviewed changes

natasha41575 approved these changes Mar 7, 2023

View reviewed changes

droot added 11 commits March 7, 2023 19:30

rollouts: Add support for GitLab as package source

eb03a0d

added build tags for github/gitlab tests

d2009f7

wired HTTPURL properly

11fd9ce

handled error in listing clusters

a0b6fb0

misc fixes

6990dee

rebased and borrowed a panic fix from an existing PR

cba9bda

rollouts: some final refinements

e2dbe3b

rollouts: addressed review comments

9c26631

updated copyright with latest year

23f33e4

rollouts: addressed Chris's comments

5416499

fixed nil pointer exception in sync template

cf781e8

droot force-pushed the rollouts-gitlab-support branch from 2c932a9 to cf781e8 Compare March 7, 2023 21:59

droot merged commit c6b840a into kptdev:main Mar 7, 2023

This was referenced Mar 8, 2023

rollouts: RSync naming schemes leads to failure for long git repo/org names. #3854

Open

Package Discovery module for Github source for rollouts #3771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollouts gitlab support #3853

Rollouts gitlab support #3853

droot commented Mar 2, 2023 •

edited

Loading

droot commented Mar 3, 2023

droot commented Mar 3, 2023

natasha41575 left a comment

natasha41575 Mar 3, 2023

droot Mar 6, 2023

natasha41575 Mar 3, 2023

droot Mar 6, 2023

natasha41575 Mar 7, 2023

natasha41575 Mar 3, 2023

droot Mar 6, 2023

natasha41575 Mar 3, 2023

droot Mar 6, 2023

natasha41575 Mar 3, 2023

droot Mar 6, 2023

droot left a comment

droot Mar 6, 2023

droot Mar 6, 2023

droot commented Mar 6, 2023

ChristopherFry left a comment

ChristopherFry Mar 6, 2023

ChristopherFry Mar 6, 2023

droot Mar 7, 2023

droot commented Mar 7, 2023

natasha41575 Mar 7, 2023 •

edited

Loading

droot Mar 7, 2023

Rollouts gitlab support #3853

Rollouts gitlab support #3853

Conversation

droot commented Mar 2, 2023 • edited Loading

droot commented Mar 3, 2023

droot commented Mar 3, 2023

natasha41575 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Mar 6, 2023

ChristopherFry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Mar 7, 2023

natasha41575 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Mar 2, 2023 •

edited

Loading

natasha41575 Mar 7, 2023 •

edited

Loading