Support all three config modes #253

jrryjcksn · 2021-06-06T19:29:40Z

What changed?

We no longer create a wego repository
- no more "private" flag
We have three automation config options:
- none (only place automation in cluster; no persistent storage)
- URL (place automation in external repository -- like our old default but no auto-creation),
- "" (use the user's app repository)
Dry run test correctly handles new git client

Why?
This is the design favored by the CTO that she will demo at GitOps days

How did you test it?

Tested locally
Added unit tests

Release notes
N/A

Documentation Changes
Yes, the getting started guide will need to be updated

pzfreo · 2021-06-07T08:22:44Z

I have concerns about the --app-config-url name.

I would prefer --gitops-config-location or --gitops-automation-location

@markramm thoughts?

luizbafilho · 2021-06-07T14:19:58Z

pkg/cmdimpl/add_test.go

+}
+
+var ignoreGitClient = gitfakes.FakeGit{
+	CloneStub: func(ctx context.Context, a, b, c string) (bool, error) {


Unless you explicitly want to see the print, there is no set all methods, it defaults to noop

luizbafilho · 2021-06-07T14:21:12Z

pkg/cmdimpl/add_test.go

@@ -171,17 +171,75 @@ func handleGitLsRemote(arglist ...interface{}) ([]byte, []byte, error) {
 	return nil, nil, fmt.Errorf("NO!")
 }

-var fakeGitClient = gitfakes.FakeGit{}
+var failGitClient = gitfakes.FakeGit{
+	CloneStub: func(ctx context.Context, a, b, c string) (bool, error) {


a failure would probably be returning an error instead of exiting the process.

This is so that any use of the code anywhere downstream will fail the test w/o trying to propagate the failure to ensure dry run is working. Returning an error wouldn't work.

the use of invalid/non-existent repo addresses would probably accomplish the same, without requiring you to implement those, but I'm fine with it. It's up to you.

That makes sense; I was just trying to be consistent across all the overrides and that was the simplest way. Not wedded to it :-)

luizbafilho · 2021-06-07T14:22:51Z

pkg/cmdimpl/add_test.go

@@ -216,7 +319,7 @@ var _ = Describe("Dry Run Add Test", func() {
 			_ = override.WithOverrides(
 				func() override.Result {
 					deps := &AddDependencies{
-						GitClient: &fakeGitClient,
+						GitClient: &failGitClient,


Just to exemplify the previous comment, this could be just GitClient: &gitfakes.FakeGit{}

Not for dry run

luizbafilho · 2021-06-07T14:29:34Z

pkg/cmdimpl/add.go

-
-		_, err = fluxops.CallFlux(cmd)
+func getUrls(client git.Git, remote string) ([]string, error) {
+	if _, ok := client.(*git.GoGit); ok {


If you do this you won't be able to mock it in the test. It will ignore it every time.

With this, you should be able to mock that

GitClient: &gitfakes.FakeGit{ OpenStub: func(s string) (*gogit.Repository, error) { r, err := gogit.Init(memory.NewStorage(), memfs.New()) Expect(err).ShouldNot(HaveOccurred()) _, err = r.CreateRemote(&config.RemoteConfig{ Name: "origin", URLs: []string{"http://foo/foo.git"}, }) Expect(err).ShouldNot(HaveOccurred()) return r, nil }, },

Without a remote, everything panicked. I had no way to fake the remote at the time so I upleveled it. I'd be very happy to do something better later

sounds good

palemtnrider

LGTM - Just a handful of nits you can choose to make or not.

palemtnrider · 2021-06-07T21:49:56Z

cmd/wego/add/cmd.go

@@ -38,8 +38,8 @@ func init() {
 	Cmd.Flags().StringVar(&params.Branch, "branch", "main", "Branch to watch within git repository")
 	Cmd.Flags().StringVar(&params.DeploymentType, "deployment-type", "kustomize", "deployment type [kustomize, helm]")
 	Cmd.Flags().StringVar(&params.PrivateKey, "private-key", filepath.Join(os.Getenv("HOME"), ".ssh", "id_rsa"), "Private key that provides access to git repository")
+	Cmd.Flags().StringVar(&params.AppConfigUrl, "app-config-url", "", "URL of external repository (if any) which will hold automation manifests; NONE to store only in the cluster")


Nit - "URL of external repository" -> "URL of an external repository"

palemtnrider · 2021-06-07T21:56:21Z

test/acceptance/test/add_tests.go

@@ -32,13 +31,12 @@ var _ = Describe("Weave GitOps Add Tests", func() {
 		defaultSshKeyPath := os.Getenv("HOME") + "/.ssh/id_rsa"
 		addCommand := "app add . "
 		appRepoName := "wego-test-app-" + RandString(8)
-		wegoRepoName := getClusterName() + "-wego"
-		appName := wegoRepoName + "-" + appRepoName
+		appName := appRepoName


NIt: It string says command is wego add and should bewego app add

palemtnrider · 2021-06-07T22:22:31Z

pkg/cmdimpl/add.go

 	if err != nil {
-		return wrapError(err, "could not get flux repo name")
+		return wrapError(err, fmt.Sprintf("could not create GitOps automation for '%s'", params.Name))


error message is using a different value than what is passed to generateApplicationGoat

pkg/cmdimpl/add.go

palemtnrider · 2021-06-07T22:24:54Z

pkg/cmdimpl/add.go

+	if err := writeAppYaml(gitClient, appYaml, ".wego"); err != nil {
+		return err
+	}
+	if err := writeGoats(gitClient, ".wego", applicationGoat); err != nil {


NIt: Since writeAppYaml and writeGoats are performing similar operations, I'd prefer to have the arguments in the same order. i.e. base path then the manifests.

pkg/cmdimpl/add.go

palemtnrider · 2021-06-07T22:32:43Z

pkg/cmdimpl/add.go

-		if err != nil {
-			return wrapError(err, "could not generate wego source manifest")
-		}
+func generateSource(repoName, repoUrl string, sourceType SourceType) ([]byte, error) {


We aren't using sourceType in the function. (I suspect it's because we have a different PR that will add the helm support)

jrryjcksn marked this pull request as ready for review June 6, 2021 22:49

pzfreo mentioned this pull request Jun 7, 2021

updated for new goat location default weaveworks/weave-gitops-docs#16

Merged

luizbafilho reviewed Jun 7, 2021

View reviewed changes

jrryjcksn force-pushed the support-all-three-config-modes branch from ccf5870 to 1c91135 Compare June 7, 2021 17:41

luizbafilho self-requested a review June 7, 2021 21:58

luizbafilho approved these changes Jun 7, 2021

View reviewed changes

weave-e2e-quickstart added 20 commits June 7, 2021 16:16

rough draft

252cdcc

merge upstream cleanup

ca2ab7b

remove .git from app config url

4227cfd

more testing of external

21f5346

external repo working

07f377b

user repo working

2553ef5

update tests for new git client and add test for generateSource

14989c0

fix lint errors

a0c9640

coverage for empty URL and non-default path

1202611

fix overzealous lint check

d0cd52b

coverage for non-user-repo add paths

27ddf33

add test for all config modes no dry-run

2d2cd83

more lint

4f35d82

remove unused function

19720f0

more no longer used code

ab575c4

first attempt at fixing smoke tests

29de258

test tweaks

c781d15

test lint

cce9898

dead code in tests

b928ccc

fix status, helmrelease naming, and kustomize naming

05b250e

weave-e2e-quickstart added 8 commits June 7, 2021 16:25

reduce refresh intervals

2878ecd

update interval in add test

788bbe7

disambiguate kustomizations

336829a

experimentally increase pod timeout for test

355c4ef

disambiguate target kustomizations

d060125

remove uses of --private flag for now since we no longer create repos

6282d33

wait for correct source in test (repo, rather than app)

69db4ff

lint error

32a49f6

palemtnrider approved these changes Jun 7, 2021

View reviewed changes

update with new helm repository changes

5064b05

jrryjcksn force-pushed the support-all-three-config-modes branch from 72bedc6 to 5064b05 Compare June 7, 2021 23:17

weave-e2e-quickstart added 2 commits June 7, 2021 17:19

remove dead code

0d31108

turn off audit for now

368a419

jrryjcksn merged commit 233d9b5 into main Jun 7, 2021

jrryjcksn deleted the support-all-three-config-modes branch June 7, 2021 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support all three config modes #253

Support all three config modes #253

jrryjcksn commented Jun 6, 2021

pzfreo commented Jun 7, 2021

luizbafilho Jun 7, 2021

luizbafilho Jun 7, 2021

jrryjcksn Jun 7, 2021

luizbafilho Jun 7, 2021

jrryjcksn Jun 7, 2021

luizbafilho Jun 7, 2021

jrryjcksn Jun 7, 2021

luizbafilho Jun 7, 2021

jrryjcksn Jun 7, 2021

luizbafilho Jun 7, 2021

palemtnrider left a comment

palemtnrider Jun 7, 2021

palemtnrider Jun 7, 2021

palemtnrider Jun 7, 2021

palemtnrider Jun 7, 2021

palemtnrider Jun 7, 2021

Support all three config modes #253

Support all three config modes #253

Conversation

jrryjcksn commented Jun 6, 2021

pzfreo commented Jun 7, 2021

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

palemtnrider 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