Remove GetProfiles and GetProfilesValues from enterprise #391

Skarlso · 2021-12-17T12:35:41Z

Closes #311

Since 0.6.0 has merged the endpoints, I expect things to just work once this code is removed. What I'm not entirely sure about is the Profiles part on the frontend and the Chart values in the CreatePullRequest section. Since none of the backing code is there, I removed that. Is that okay?

Note: I'm trying to test this locally now, but it's hard to get all the infrastructure up and running.

Skarlso · 2021-12-17T13:44:33Z

Failure doesn't seem related:

=== RUN   TestCreatePullRequestInGitHubOrganization
    git_test.go:81: 
        	Error Trace:	git_test.go:81
        	Error:      	Received unexpected error:
        	            	unable to write files to branch "feature-01": unable to create new branch "feature-01" from commit "047b51d4c38cdb1e5734c4adfef30489269c8472" in branch "main": POST https://api.github.com/repos/***/capi-server-test-repo-508/git/refs: 409 Git Repository is empty. []
        	Test:       	TestCreatePullRequestInGitHubOrganization
--- FAIL: TestCreatePullRequestInGitHubOrganization (2.62s)

cmd/clusters-service/app/server.go

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

Skarlso · 2022-01-04T14:00:15Z

cmd/clusters-service/app/server.go

@@ -101,7 +105,10 @@ func StartServer(ctx context.Context, log logr.Logger, tempDir string) error {
 		sourcev1beta1.AddToScheme,
 	}
 	schemeBuilder.AddToScheme(scheme)
-	kubeClientConfig := config.GetConfigOrDie()
+	kubeClientConfig, err := config.GetConfig()


Added this because I run into a problem where this was erroring out, but didn't return any kind of error message at all. Because the underlying logger for config is not initialised properly, thus it was pushing the log message into the void.

Skarlso · 2022-01-04T16:39:33Z

The test failure is probably me somehow. Something must still be missing. Maybe the UI also needs to be rebound somewhere? I don't know, I need someone to review this who has better understanding of enterprise and the UI.

foot · 2022-01-04T18:20:42Z

The test failure is probably me somehow. Something must still be missing. Maybe the UI also needs to be rebound somewhere? I don't know, I need someone to review this who has better understanding of enterprise and the UI.

We have seen the panic in other PRs today actually so might be something has changed somewhere

Skarlso · 2022-01-05T11:57:08Z

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

 	wegogit "github.com/weaveworks/weave-gitops/pkg/git"
 	"github.com/weaveworks/weave-gitops/pkg/server/middleware"
 	"google.golang.org/genproto/googleapis/api/httpbody"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 	grpcStatus "google.golang.org/grpc/status"
+	"gopkg.in/yaml.v2"


Fuuuuuu 😡

Skarlso · 2022-01-05T12:40:04Z

Woooot all green 🟢 Also tested manually, it seems to be okay, but I'm getting a weird url problem when it's trying to download the profile index file?

{"level":"info","ts":1641376180.152812,"caller":"middleware/middleware.go:93","msg":"invalid token format"}
E0105 10:49:45.160102   36569 middleware.go:40]  "msg"="server error" "error"="failed to scan HelmRepository \"wego-system\"/\"weaveworks-charts\" for charts: fetching profiles from HelmRepository weaveworks-charts/wego-system: error fetching index file: Get \"http://source-controller.wego-system.svc.cluster.local./helmrepository/wego-system/weaveworks-charts/index.yaml\": dial tcp: lookup source-controller.wego-system.svc.cluster.local.: no such host"

Not yet sure what to do about this.

foot · 2022-01-06T08:39:38Z

Not yet sure what to do about this.

Are you getting this error locally? Could be you need to set SOURCE_CONTROLLER_LOCALHOST, https://github.com/weaveworks/weave-gitops-enterprise/pull/374/files

Skarlso · 2022-01-06T08:49:42Z

@foot Yep, locally. :) Ok, cool, I'll try to set that.
I'm running it like this:

DB_URI=/tmp/wge.db CAPI_CLUSTERS_NAMESPACE=default CAPI_TEMPLATES_NAMESPACE=default GIT_PROVIDER_TYPE=github GIT_PROVIDER_HOSTNAME=github.com CAPI_TEMPLATES_REPOSITORY_URL=https://github.com/my-org/my-repo CAPI_TEMPLATES_REPOSITORY_BASE_BRANCH=main ENTITLEMENT_SECRET_NAMESPACE=wego-system ENTITLEMENT_SECRET_NAME=weave-gitops-enterprise-credentials go run cmd/clusters-service/main.go

And indeed, this does not set SOURCE_CONTROLLER_LOCALHOST, and neither does it set RUNTIME_NAMESPACE for the record.

Skarlso · 2022-01-07T09:23:18Z

@foot I'm still getting the same even after doing the steps in that Dev doc. In any case, I'm fairly confident that this thing is working, but I can't manually verify it on my machine. :/

cmd/clusters-service/app/options.go

aclevername · 2022-01-10T09:32:52Z

cmd/clusters-service/app/server.go

+	helmRepoName := viper.GetString("profile-helm-repository")
+	helmRepoNamespace := os.Getenv("RUNTIME_NAMESPACE")


do we need to check if either are empty and error appropriately? Although I'm guessing helmRepoName has a default value here?

We might pick a default for it somewhere but it really should be clearer. It would be nice to start the service locally without have to declare 10 env vars.

We might pick a default for it somewhere but it really should be clearer. It would be nice to start the service locally without have to declare 10 env vars.

+100 definitely. In core it uses a flag for the namespace instead of an environment variable (although a combination of both using viper is even better 😄 ). Perhaps worth migrating to that, then it just all gets passed around from the cmd

foot · 2022-01-10T09:33:21Z

@foot I'm still getting the same even after doing the steps in that Dev doc. In any case, I'm fairly confident that this thing is working, but I can't manually verify it on my machine. :/

Yeah! 🤔 the SOURCE_CONTROLLER_LOCALHOST hack didn't get migrated to the core repo so might not be working w/ the new routes. We were looking into re-adding it in https://github.com/weaveworks/weave-gitops/pull/1277/files#diff-6ce67b082d0e02ad66ea67bd9e7d1a8c9d5d6c1144501f95299fcdb67106f058R235 but did you guys test out alternative solutions there like this telemetry dev service? cc @aclevername

aclevername · 2022-01-10T09:33:28Z

cmd/clusters-service/app/server.go

@@ -137,6 +147,7 @@ func StartServer(ctx context.Context, log logr.Logger, tempDir string) error {
 			Namespace: os.Getenv("CAPI_TEMPLATES_NAMESPACE"),
 		}),
 		WithApplicationsConfig(appsConfig),
+		WithProfilesConfig(profilesConfig),


inline config creation maybe? And rename wego_server to just core or wego? Its ugly/long atm

¯_(ツ)_/¯ I have no opinion on this. I was following appsConfig style. I can do that too.

aclevername · 2022-01-10T09:34:36Z

docs/developing.md

@@ -4,6 +4,43 @@

 A guide to making it easier to develop `weave-gitops-enterprise`. If you came here expecting but not finding an answer please make an issue to help improve these docs!

+## Building the project


aclevername · 2022-01-10T09:35:54Z

cmd/clusters-service/app/server.go

+	if err := capi_proto.RegisterClustersServiceHandlerServer(ctx, mux, server.NewClusterServer(
+		args.Log,
+		args.TemplateLibrary,
+		args.GitProvider,
+		args.KubernetesClient,
+		args.DiscoveryClient,
+		args.Database,
+		args.CAPIClustersNamespace,
+		args.ProfileHelmRepository,
+		args.HelmRepositoryCacheDirectory,
+	)); err != nil {


can we move the NewClusterServer out? hard to read like this IMO

aclevername · 2022-01-10T09:37:57Z

Yeah! thinking the SOURCE_CONTROLLER_LOCALHOST hack didn't get migrated to the core repo so might not be working w/ the new routes. We were looking into re-adding it in https://github.com/weaveworks/weave-gitops/pull/1277/files#diff-6ce67b082d0e02ad66ea67bd9e7d1a8c9d5d6c1144501f95299fcdb67106f058R235 but did you guys test out alternative solutions there like this telemetry dev service? cc @aclevername

Not yet 😞

aclevername

The removals/additionals LGTM, but would want a 2nd reviewer before merging 😄

foot · 2022-01-10T09:47:55Z

cmd/clusters-service/pkg/capi/parse_test.go

 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/scheme"
+
+	capiv1 "github.com/weaveworks/weave-gitops-enterprise/cmd/clusters-service/api/v1alpha1"


Nice! I just let vscode/gofmt take care of this. Will gofmt mess this up?

Nope. gofmt is a bit "dumb" it will sort anything that is in a single block. The further organisation that I'm doing is "manual". I'm following this:

standard library

third party

organisational ( inside weaveworks )

package local

I'm doing this via goland. :) Which can have these settings and autoformats. gofmt will just sort any distinctive groups.

foot · 2022-01-10T09:48:27Z

docs/developing.md

+make GITHUB_BUILD_TOKEN=${GITHUB_TOKEN}
+```
+
+Further, don't forget to update your `~/.gitconfig` with:


foot

LGTM! There is some library code duplication e.g. core/ee each have an impl. of fetchIndexFile right now, not the end of the world at all, but nice if core exported a go api for using profiles from other endpoints too, wdyt? We can do this in another PR though.

Nice one! 🎉

Skarlso · 2022-01-10T10:03:29Z

@foot Definitely, I agree, including the observation made by Jake here #391 (comment)

Extracting this to include mux then I can throw out the whole initialise part of that segment. :)

Remove GetProfiles and GetProfilesValues from enterprise

9163ff1

Skarlso added severity_low team/pitch-black enhancement New feature or request labels Dec 17, 2021

Skarlso added 2 commits December 17, 2021 13:50

removed charts

7e1bb1d

Removing more stuff

d54f9f5

Skarlso requested review from yiannistri, bigkevmcd, AlinaGoaga, aclevername and nikimanoledaki December 17, 2021 13:57

Updated developer docs

3f37fa2

Skarlso commented Dec 20, 2021

View reviewed changes

cmd/clusters-service/app/server.go Outdated Show resolved Hide resolved

Skarlso marked this pull request as ready for review December 20, 2021 09:45

Added routing to the profiles server to wego-core

4dd9c41

Skarlso force-pushed the link_moved_endpoints branch from 999531e to 4dd9c41 Compare December 20, 2021 09:51

Skarlso commented Dec 20, 2021

View reviewed changes

cmd/clusters-service/app/server.go Outdated Show resolved Hide resolved

Skarlso commented Dec 20, 2021

View reviewed changes

cmd/clusters-service/app/server.go Outdated Show resolved Hide resolved

aclevername reviewed Dec 20, 2021

View reviewed changes

cmd/clusters-service/pkg/server/server.go Outdated Show resolved Hide resolved

Skarlso added 3 commits January 4, 2022 11:58

Merge branch 'main' into link_moved_endpoints

d9f4894

Updated with putting back profile-helm-repository

fd3bc20

Updated modules

fb6de1a

Skarlso requested a review from foot January 4, 2022 11:56

Added error return statement to getting the kube config

efc4382

Skarlso commented Jan 4, 2022

View reviewed changes

Skarlso force-pushed the link_moved_endpoints branch 2 times, most recently from 48bbeab to 46c90d9 Compare January 5, 2022 09:34

But back charts and loading of profile files and data

e574928

Skarlso force-pushed the link_moved_endpoints branch from 46c90d9 to e574928 Compare January 5, 2022 11:37

Skarlso commented Jan 5, 2022

View reviewed changes

Found incorrectly imported yaml library

00d9026

Skarlso added 3 commits January 6, 2022 09:49

Merge branch 'main' into link_moved_endpoints

582def1

Merge branch 'main' into link_moved_endpoints

62e450c

Merge branch 'main' into link_moved_endpoints

de09ffd

Skarlso force-pushed the link_moved_endpoints branch from f2dc912 to de09ffd Compare January 6, 2022 16:06

aclevername reviewed Jan 10, 2022

View reviewed changes

cmd/clusters-service/app/options.go Show resolved Hide resolved

aclevername reviewed Jan 10, 2022

View reviewed changes

aclevername approved these changes Jan 10, 2022

View reviewed changes

foot reviewed Jan 10, 2022

View reviewed changes

docs/developing.md

make GITHUB_BUILD_TOKEN=${GITHUB_TOKEN}

```

Further, don't forget to update your `~/.gitconfig` with:

Copy link

Collaborator

foot Jan 10, 2022

Choose a reason for hiding this comment

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

🎉

foot approved these changes Jan 10, 2022

View reviewed changes

Renamed import, inlined config and extracted cluster creation

2d8e0d6

Skarlso merged commit 66a0bc7 into main Jan 10, 2022

Skarlso deleted the link_moved_endpoints branch January 10, 2022 12:24

		helmRepoName := viper.GetString("profile-helm-repository")
		helmRepoNamespace := os.Getenv("RUNTIME_NAMESPACE")

		@@ -4,6 +4,43 @@

		A guide to making it easier to develop `weave-gitops-enterprise`. If you came here expecting but not finding an answer please make an issue to help improve these docs!

		## Building the project

Remove GetProfiles and GetProfilesValues from enterprise #391

Remove GetProfiles and GetProfilesValues from enterprise #391

Uh oh!

Conversation

Skarlso commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Skarlso commented Dec 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skarlso commented Jan 4, 2022

Uh oh!

foot commented Jan 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skarlso commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foot commented Jan 6, 2022

Uh oh!

Skarlso commented Jan 6, 2022

Uh oh!

Skarlso commented Jan 7, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foot commented Jan 10, 2022

Uh oh!

aclevername Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aclevername commented Jan 10, 2022

Uh oh!

aclevername left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skarlso commented Jan 10, 2022

Uh oh!

Uh oh!

Skarlso commented Dec 17, 2021 •

edited

Loading

Skarlso commented Jan 5, 2022 •

edited

Loading

aclevername Jan 10, 2022 •

edited

Loading

foot left a comment •

edited

Loading