Add functionality to gitops beta run command #2411

opudrovs · 2022-07-07T09:23:52Z

opudrovs · 2022-07-07T10:04:40Z

It's a WIP, will clean up error handling, vars, and comments later. This is a prototype code. It works in general (checks, connection to cluster and if Flux is installed and if not it installs Flux), but I'm not sure if it works correctly.

Based on step-by-step @ozamosi 's suggestions:
#2355 (comment)

opudrovs · 2022-07-07T11:02:57Z

cmd/gitops/beta/run/cmd.go

@@ -50,6 +74,266 @@ func betaRunCommandPreRunE(endpoint *string) func(*cobra.Command, []string) erro

 func betaRunCommandRunE(opts *config.Options, client *resty.Client) func(*cobra.Command, []string) error {
 	return func(cmd *cobra.Command, args []string) error {
+		// If there is no cluster in the kube config, return an error.
+		cfg, ctx, err := kube.RestConfig()


Does using RestConfig work equivalently/errs the same way to the outOfClusterConfig (@ozamosi suggested) in our case?

Or do I need to copy-paste/expose code from the outOfClusterConfig?

How is InClusterConfig different from outOfClusterConfig, why int he RestConfig we first try to create the first and if it fails the second one?

They're different in that InClusterConfig connects to the kube API as if it's running inside the cluster, and outOfClusterConfig will connect as if it's running outside the cluster. This is a legacy of how all code used to work whether you called it from the web API or the CLI.

We expect this to run outside the cluster, so as far as I know InClusterConfig won't do anything, but it might end up returning a different error message in the end? But since the error you return is ErrGetKubeClient, it doesn't matter.

This should work fine 👍

opudrovs · 2022-07-07T11:06:09Z

cmd/gitops/beta/run/cmd.go

+		}
+
+		// If there is a valid connection to a cluster when the command is run, connect to the currently selected cluster.
+		kubeClient, err := kube.NewKubeHTTPClientWithConfig(cfg, ctx)


I am creating a client to list objects simply, like that, and then reuse it (by passing it to installFlux, and thus apply and newManager).

Do I need to create it in a more complex way with more settings, KubeConfig, like in the Flux code?

I frankly don't know - I would use the same client, unless we discover a reason not to.

opudrovs · 2022-07-07T11:09:58Z

cmd/gitops/beta/run/cmd.go

+			Kind:    "Namespace",
+		})
+
+		ctx2 := context.Background()


The context I have from creating the RestConfig is a string. So, I need another context here. Is it OK to use context.Background() for getting the list of objects and passing it to installFlux? Finally, it will be passed to ApplyAll.

You can use context.Background() because we would not need to set its deadline or cancel it.

The context I have from creating the RestConfig is a string

Yeah, that context seems to be a clusterName rather than a context in the context module sense - I would not call that string "context", because I find it confusing.

Is it OK to use context.Background() for getting the list of objects and passing it to installFlux?

Yes. Contexts are about describing a "work unit" across threads - so you have a way to connect e.g. which web request caused a server to run which database queries, if that makes sense.

The "work unit" here is the command itself, so the command's context is the right one to attach to any other work the command generates.

opudrovs · 2022-07-07T11:10:56Z

cmd/gitops/beta/run/cmd.go

+		ctx2 := context.Background()
+
+		listOptions := ctrlclient.MatchingLabels{
+			coretypes.PartOfLabel: "flux",


Is it OK to use coretypes here in a CLI command?

opudrovs · 2022-07-07T11:21:00Z

cmd/gitops/beta/run/cmd.go

+		return fmt.Errorf("couldn't create file %+v", err)
+	}
+
+	err = os.WriteFile(filepath.Join(filePath, shortFilename), content, 0666)


Am I making the dir and writing the file with the correct permission?

And should I use the gitops command directory or some working directory. If the latter, how should I get it. (I think I heard smth. mentioned about the working directory during the last standup but it was about another issue.)

Besides, do we still need to write the file if we are not actually reading the file for applying it to the cluster and are just passing the manifest's content (which is in memory in a var) to apply.

Or do we need to read the manifest contents from file/will the file be read later?

Am I making the dir and writing the file with the correct permission?

Let's use 0775 and 0664 to remove world write permissions.

And should I use the gitops command directory or some working directory

Eventually, we need logic to find the root git directory and use that as a default, then add a command line option to override that, and then use that directory. But that's out of scope for this PR.

I think using os.Getwd here is enough to demonstrate it works.

do we still need to write the file if we are not actually reading the file

Yes - when you've finished running this command, you should be able to just run git commit -a; git push and have that give you the same git repository that flux bootstrap would have.

hmm. I think this behavour + the locaton of Flux's manifest need refinement.

opudrovs · 2022-07-07T11:27:09Z

cmd/gitops/beta/run/cmd.go

+	// kubeclientOptions.BindFlags(rootCmd.PersistentFlags())
+
+	// rootCmd.RegisterFlagCompletionFunc("context", contextsCompletionFunc)
+	// rootCmd.RegisterFlagCompletionFunc("namespace", resourceNamesCompletionFunc(corev1.SchemeGroupVersion.WithKind("Namespace")))


What should I do with global flags (if any of those supported by the gitops command are provided) in the run command here? I've just copy-pasted and commented out code from the Flux instal command.

Which of this or original gitops flags do I need to add here?

Oh god, do I have to make decisions 😱

So, I looked at what flags flux install --help and gitops --help prints out and tried to look at what's important Note: this is written without looking at the code, so if I say something that is not fairly straight-forward copy-paste, then the correct answer is "that's very interesting Robin, I suggest you make a ticket if you feel strongly about it, because that's not part of this ticket's description".

--version makes sense - especially as that'd let us set and print a default in the help text, so it's possible to figure out what we'd do. We should have a default set at compiletime in pkg/version/version.go already. Maybe it should be called --flux-version instead here, though.

I think maybe --components-extra and --components would be useful as well, again as a bit of documentation in the help text what the thing will do.

There's already a global --namespace in gitops, so we should use that here - especially since I've come to grow opinions about namespaces and gitops recently 😊

We need a timeout set to something or install doesn't work, and what I hard-coded isn't a good timeout. Let's just add a --timeout flag?

--insecure-skip-tls-verify is a global arg in both gitops and flux, is it easy to hook up?

The rest of the global gitops flags are irrelevant.

There's loads of global flux flags which I'm sure are important to some people (personally, I really want --context), but they all require changes to the kube config/client bits, so I think they're out of scope for now.

opudrovs · 2022-07-07T11:28:24Z

cmd/gitops/beta/run/cmd.go

+
+	var kubeconfigArgs = genericclioptions.NewConfigFlags(false)
+
+	var kubeclientOptions = new(runclient.Options)


I am not sure should configure kubeconfigArgs and kubeclientOptions here. This is code I copy-pasted from Flux and simplified.

What exactly should I configure here for our purpose?

opudrovs · 2022-07-07T11:35:55Z

cmd/gitops/beta/run/cmd.go

+		return fmt.Errorf("couldn't write flux manifests to file %+v", err)
+	}
+
+	rf := rootFlags{


Will remove everything related to root flags, copy-pasted and I think here this code is not needed, we already used called MakeDefaultOptions when generating the manifests.

opudrovs · 2022-07-07T11:36:09Z

cmd/gitops/beta/run/cmd.go

+	return nil
+}
+
+type rootFlags struct {


Will be removed.

opudrovs · 2022-07-07T11:43:48Z

cmd/gitops/beta/run/cmd.go

+}
+
+func newManager(kubeClient ctrlclient.Client, rcg genericclioptions.RESTClientGetter) (*ssa.ResourceManager, error) {
+	restMapper, err := rcg.ToRESTMapper()


See above the question about the kube client. Can I re-use the passed kube client here or should I create a new config and client here, like in the Flux code in func newManager?

opudrovs · 2022-07-07T11:48:51Z

cmd/gitops/beta/run/cmd.go

+	return man.WaitForSet(changeSet.ToObjMetadataSet(), ssa.WaitOptions{Interval: 2 * time.Second, Timeout: time.Minute})
+}
+
+func apply(kubeClient ctrlclient.Client, ctx context.Context, rcg genericclioptions.RESTClientGetter, opts *runclient.Options, manifestPath string, manifestContent []byte) (string, error) {


Copy-pasted, slightly simplified. Can anything be simplified here or do we need everything to apply the generated manifest to the cluster?

opudrovs · 2022-07-07T11:49:49Z

cmd/gitops/beta/run/cmd.go

+	}
+
+	if len(objs) == 0 {
+		return "", fmt.Errorf("no Kubernetes objects found at: %s", manifestPath)


Can be removed, probably. manifestPath here is used only for this error message. The generated manifest should be guaranteed to be valid?

opudrovs · 2022-07-07T18:16:29Z

@ozamosi great points, thank you! 🎉 Will rework the code based on yours and @chanwit 's suggestions.

chanwit · 2022-07-08T12:49:56Z

cmd/gitops/beta/run/cmd.go

+		if fluxVersion == "" {
+			filePath := args[0]
+
+			err = run.InstallFlux(log, ctx2, kubeClient, filepath.Join(filePath, run.FluxDirectory), run.ShortManifestFilename)


I think we have to discuss the semantics of this flux location.

For example,
When running gitops beta run ./deploy/overlay/dev, the filePath variable would be ./deploy/overlay/dev.

Then the InstallFlux function would install Flux manifests to ./deploy/overlay/dev/flux-system, which would not be the correct location.

Per Flux convention, an on-disk location of Flux installation would be something like
./cluster/dev/flux-system - which is normally passed to the flux bootstrap command.

So I think we need to refine this behavior.

Start adding prototype code.

Add generating Flux manifests if Flux is not installed.

Fix error messages when getting the Flux version.

Remove comments.

Rework error message in the `run` command.

opudrovs · 2022-07-11T02:00:44Z

@ozamosi @chanwit I've reworked code based on your suggestions.

Reworked the error/warning messages to use the logger when appropriate (sometimes a regular formatted error is returned to avoid duplicate error messages). Please let me know if message wording should be corrected or if I missed some places where it is better to use a logger alongside with returning an error.
for kube config options and kube client options I copied code from Flux and simplified it. Maybe we don't need all the options.
For now, I've removed everything related to global flags: I think it is better to add that functionality in a separate issue/PR in order not to hold @chanwit 's background part of the sync component PR and to check the basic functionality first. If you think that support for global flags should be added in this PR, please let me know.
The same about tests for the run command: I haven't added them for now, haven't found a relevant sample test which can reworked fast, so I would need some time to think on how to add the tests because the command is rather complex.
Also moved all code related to getting the Flux version for the version endpoint within the correspondinggetFluxVersion function of the version endpoint for consistency with the run cmd. Fixed an error message in the getFluxVersion function in the version endpoint, which was copy/pasted from getting Kubernetes version (get server version was not relevant to getting flux version).

Testing:

Tested the code against kind and k3d (with @ozamosi 's help 🙂 ) clusters. How should I test it against a remote cluster (which is in the ticket acceptance criteria)?
To me it looks that the code installs all Flux components required in the acceptance criteria, but if any components are missing please let me know.

Fix error message in the `getFluxVersion` function in the version endpoint.

ozamosi

Your description of what is missing and will be done later matches what I'd want. It's in beta, so it's not the end of the world if the code in main isn't perfect yet.

The code here looks like it'll make a good starting point for the rest of the work, so

opudrovs · 2022-07-11T11:00:51Z

@ozamosi great, thanks!

chanwit · 2022-07-11T11:25:10Z

pkg/run/run.go

+		Timeout:      5 * time.Second,
+	}
+
+	manifest, err := install.Generate(opts, "")


This variable would be manifests to be consistent with your error logs

Fixed, good catch!

chanwit

LGTM thank you @opudrovs 🥇

opudrovs · 2022-07-11T11:51:34Z

@chanwit thank you! 🎉

opudrovs added team/denim type/enhancement New feature or request labels Jul 7, 2022

opudrovs commented Jul 7, 2022

View reviewed changes

cmd/gitops/beta/run/cmd.go Outdated

return nil

}

type rootFlags struct {

Copy link

Contributor Author

opudrovs Jul 7, 2022

Choose a reason for hiding this comment

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

Will be removed.

opudrovs commented Jul 7, 2022

View reviewed changes

opudrovs requested review from chanwit and ozamosi July 7, 2022 12:19

opudrovs force-pushed the 2355-add-functionality-to-gitops-beta-run-command branch 4 times, most recently from 14ff86e to 4c3aa52 Compare July 8, 2022 12:27

chanwit mentioned this pull request Jul 8, 2022

implement dev bucket installation #2416

Merged

chanwit reviewed Jul 8, 2022

View reviewed changes

opudrovs force-pushed the 2355-add-functionality-to-gitops-beta-run-command branch 2 times, most recently from 09adb0f to 786ff90 Compare July 8, 2022 14:06

opudrovs added 4 commits July 10, 2022 16:29

Add run command outline.

59d23a8

Add command outline.

825ffdd

Start adding prototype code.

Add getting the Flux version.

1d0428a

Update packages to enable Flux manifest generation.

251effa

Add generating Flux manifests if Flux is not installed.

opudrovs added 8 commits July 10, 2022 16:29

Add writing Flux manifest content to file.

e03a325

Add prototype code installing Flux on the selected cluster.

0a468d5

Fix error messages when getting the Flux version.

Move run command functions to pkg/run.

9f0a467

Add passing logger, context, and client to all pkg/run functions.

9f520e2

Remove storing the manifest on disk for now.

df72bc1

Rename the context var.

fdbebae

Remove comments.

Remove the placeholder for run command flags.

16f1954

Rework error message in the `run` command.

Rework more error messages.

4e3d6d2

opudrovs force-pushed the 2355-add-functionality-to-gitops-beta-run-command branch from 120b1da to 4e3d6d2 Compare July 10, 2022 21:26

opudrovs added 2 commits July 11, 2022 00:31

Move configuring client config and kube client to separate functions.

5b4549e

Remove mentions of global flags from the run command for now.

ae1d9a6

opudrovs force-pushed the 2355-add-functionality-to-gitops-beta-run-command branch from 6497310 to ae1d9a6 Compare July 10, 2022 23:18

opudrovs requested a review from chanwit July 11, 2022 02:01

opudrovs marked this pull request as ready for review July 11, 2022 02:01

Rework messages in the run command.

63aae63

Fix error message in the `getFluxVersion` function in the version endpoint.

opudrovs force-pushed the 2355-add-functionality-to-gitops-beta-run-command branch from d20b805 to 63aae63 Compare July 11, 2022 02:15

ozamosi approved these changes Jul 11, 2022

View reviewed changes

chanwit reviewed Jul 11, 2022

View reviewed changes

chanwit approved these changes Jul 11, 2022

View reviewed changes

Fix variable name for consistency.

a7372c5

opudrovs merged commit 452f4b6 into main Jul 11, 2022

opudrovs deleted the 2355-add-functionality-to-gitops-beta-run-command branch July 11, 2022 11:54

This was referenced Jul 26, 2022

Updates for 0.9.1-rc.2 #2491

Merged

Updates for 0.9.1 #2500

Merged

weave-gitops-bot mentioned this pull request Aug 10, 2022

Updates for 0.9.2-rc.1 #2578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to gitops beta run command #2411

Add functionality to gitops beta run command #2411

opudrovs commented Jul 7, 2022 •

edited

opudrovs commented Jul 7, 2022 •

edited

opudrovs Jul 7, 2022

ozamosi Jul 7, 2022

opudrovs Jul 7, 2022

ozamosi Jul 7, 2022

opudrovs Jul 7, 2022

chanwit Jul 7, 2022

ozamosi Jul 7, 2022

opudrovs Jul 7, 2022

ozamosi Jul 7, 2022

opudrovs Jul 7, 2022

opudrovs Jul 7, 2022

ozamosi Jul 7, 2022 •

edited

chanwit Jul 7, 2022

opudrovs Jul 7, 2022

ozamosi Jul 7, 2022

opudrovs Jul 7, 2022 •

edited

opudrovs Jul 7, 2022

opudrovs Jul 7, 2022

opudrovs Jul 7, 2022

opudrovs Jul 7, 2022

opudrovs Jul 7, 2022

opudrovs commented Jul 7, 2022

chanwit Jul 8, 2022

opudrovs commented Jul 11, 2022 •

edited

ozamosi left a comment

opudrovs commented Jul 11, 2022

chanwit Jul 11, 2022

opudrovs Jul 11, 2022

chanwit left a comment

opudrovs commented Jul 11, 2022


		var kubeconfigArgs = genericclioptions.NewConfigFlags(false)

		var kubeclientOptions = new(runclient.Options)

Add functionality to gitops beta run command #2411

Add functionality to gitops beta run command #2411

Conversation

opudrovs commented Jul 7, 2022 • edited

opudrovs commented Jul 7, 2022 • edited

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

ozamosi Jul 7, 2022 • edited

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

opudrovs Jul 7, 2022 • edited

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

opudrovs commented Jul 7, 2022

Choose a reason for hiding this comment

opudrovs commented Jul 11, 2022 • edited

ozamosi left a comment

Choose a reason for hiding this comment

opudrovs commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanwit left a comment

Choose a reason for hiding this comment

opudrovs commented Jul 11, 2022

opudrovs commented Jul 7, 2022 •

edited

opudrovs commented Jul 7, 2022 •

edited

ozamosi Jul 7, 2022 •

edited

opudrovs Jul 7, 2022 •

edited

opudrovs commented Jul 11, 2022 •

edited