Skip to content
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

Refactor install #1284

Merged
merged 60 commits into from
Jan 22, 2022
Merged

Refactor install #1284

merged 60 commits into from
Jan 22, 2022

Conversation

josecordaz
Copy link
Contributor

@josecordaz josecordaz commented Jan 7, 2022

Closes:

What changed?
Initial work to refactor installer

Why?
To make it more API/CLI friendly and to align with the latest architecture ADR.

How did you test it?
Added unit tests and fixed all acceptance tests related to the install command.

Release notes
N/A

Documentation Changes
N/A

@josecordaz josecordaz added the type/enhancement New feature or request label Jan 12, 2022
A function was renamed but their calls were not
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Any extra comments that you could add to the code about future plans or why certain things might seem weird would be good.

Copy link
Contributor

@palemtnrider palemtnrider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a few suggested changes and a few nits.

cmd/gitops/install/cmd.go Show resolved Hide resolved
cmd/gitops/install/cmd.go Show resolved Hide resolved
pkg/services/gitops/uninstall.go Outdated Show resolved Hide resolved
pkg/services/gitopswriter/gitopswriter.go Outdated Show resolved Hide resolved
pkg/services/gitopswriter/gitopswriter.go Outdated Show resolved Hide resolved
pkg/services/install/install.go Outdated Show resolved Hide resolved
pkg/services/install/install_test.go Outdated Show resolved Hide resolved
})

It("should fail getting default branch", func() {
fakeKubeClient.GetClusterStatusReturns(kube.Unmodified)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 8 lines appear to be repeated in quite of few of these tests, consider either move to setup or make a function that each could call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@palemtnrider Can't do that. Each test uses specific setup lines. Not all lines appear in all tests and not all are the same. Let me know if I'm missing something. This another approach where I would be duplicate any single line but seems a bit confusing. The huge advantage I see is that if I need to make a change somewhere it would be in a single place only. Let me know if you want to discuss this in a call.

Context("error paths2", func() {
		someError := errors.New("some error")
		It("should fail validating wego installation", func() {
			fakeKubeClient.GetClusterStatusReturns(kube.Unknown)

			err := installer.Install(testNamespace, configRepo, true)
			Expect(err).Should(MatchError("failed validating wego installation: Weave GitOps cannot talk to the cluster"))
		})
		Context("return unmodified on GetClusterStatus and valid wego config", func() {
			fakeKubeClient.GetClusterStatusReturns(kube.Unmodified)
			fakeKubeClient.GetWegoConfigReturns(&kube.WegoConfig{
				FluxNamespace: testNamespace,
				WegoNamespace: testNamespace,
			}, nil)
			It("should fail getting cluster name", func() {
				fakeKubeClient.GetClusterNameReturns("", someError)

				err := installer.Install(testNamespace, configRepo, true)
				Expect(err).Should(MatchError(fmt.Sprintf("failed getting cluster name: %s", someError)))
			})
			Context("return valid cluster name", func() {
				fakeKubeClient.GetClusterNameReturns(clusterName, nil)
				It("should fail installing flux", func() {
					fakeFluxClient.InstallReturns(nil, someError)

					err := installer.Install(testNamespace, configRepo, true)
					Expect(err).Should(MatchError(fmt.Sprintf("failed installing flux: %s", someError)))
				})
				Context("do not return error on flux install", func() {
					fakeFluxClient.InstallReturnsOnCall(0, nil, nil)
					It("should fail getting bootstrap manifests", func() {
						fakeFluxClient.InstallReturnsOnCall(1, nil, someError)

						err := installer.Install(testNamespace, configRepo, true)
						Expect(err).Should(MatchError(fmt.Sprintf("failed getting bootstrap manifests: failed getting runtime manifests: %s", someError)))
					})
					Context("do not return error on flux install", func() {
						fakeFluxClient.InstallReturnsOnCall(1, nil, nil)
						It("should fail getting default branch", func() {
							fakeGitProvider.GetDefaultBranchReturnsOnCall(0, "", someError)

							err := installer.Install(testNamespace, configRepo, true)
							Expect(err).Should(MatchError(fmt.Sprintf("failed getting default branch: %s", someError)))
						})
						Context("return valid default branch", func() {
							fakeGitProvider.GetDefaultBranchReturnsOnCall(0, "main", nil)
							It("should fail getting config repo git source", func() {
								fakeGitProvider.GetRepoVisibilityReturns(nil, someError)

								err := installer.Install(testNamespace, configRepo, true)
								Expect(err).Should(MatchError(fmt.Sprintf("failed getting git source: failed getting ref secret: %s", someError)))
							})
							Context("return private repo visibility", func() {
								privateVisibility := gitprovider.RepositoryVisibilityPrivate
								fakeGitProvider.GetRepoVisibilityReturns(&privateVisibility, nil)
								It("should fail applying bootstrap manifests", func() {
									fakeKubeClient.ApplyReturns(someError)

									err := installer.Install(testNamespace, configRepo, true)
									Expect(err).Should(MatchError(fmt.Sprintf("error applying manifest .weave-gitops/clusters/test-cluster/system/wego-system.yaml: %s", someError)))
								})
								Context("do not fail when applying manifests", func() {
									fakeKubeClient.ApplyReturns(nil)
									It("should fail getting gitops manifests", func() {
										fakeFluxClient.InstallReturnsOnCall(2, nil, someError)

										err := installer.Install(testNamespace, configRepo, true)
										Expect(err).Should(MatchError(fmt.Sprintf("failed generating gitops manifests: failed getting runtime manifests: %s", someError)))
									})
									Context("do not fail when calling install for second time", func() {
										fakeFluxClient.InstallReturnsOnCall(2, nil, nil)
										It("should fail writing directly to branch", func() {
											fakeGitClient.CloneReturns(false, someError)

											err := installer.Install(testNamespace, configRepo, true)
											Expect(err).Should(MatchError(fmt.Sprintf("failed writting to default branch failed to clone repo: failed cloning user repo: ssh://git@github.com/test-user/test-repo.git: %s", someError)))
										})
										It("should fail creating a pull requests", func() {
											fakeGitProvider.CreatePullRequestReturns(nil, someError)

											err := installer.Install(testNamespace, configRepo, false)
											Expect(err).Should(MatchError(fmt.Sprintf("failed creating pull request: %s", someError)))
										})
									})
								})
							})
						})
					})
				})
			})
		})
	})

pkg/services/install/install_test.go Outdated Show resolved Hide resolved
test/acceptance/test/install_tests.go Show resolved Hide resolved
# Conflicts:
#	cmd/gitops/install/cmd.go
#	pkg/services/auth/auth_test.go
Copy link
Contributor

@palemtnrider palemtnrider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

cmd/gitops/install/cmd.go Show resolved Hide resolved
@josecordaz josecordaz merged commit 3af4189 into main Jan 22, 2022
@josecordaz josecordaz deleted the refactor-install branch January 22, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants