Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add fluxctl install command #2287

Merged
merged 19 commits into from
Jul 29, 2019
Merged

Add fluxctl install command #2287

merged 19 commits into from
Jul 29, 2019

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Jul 23, 2019

Simlarly to linkerd install, fluxctl install is meant to print out and tweak the Kubernetes manifests needed to install Flux in a cluster.

Its output is intended for piping into other commands such as:

# Install Flux and make it use Git repository git@github.com:<your username>/flux-get-started
fluxctl install --git-url 'git@github.com:<your username>/flux-get-started' | kubectl apply -f -

cmd/fluxctl/setup_cmd.go Outdated Show resolved Hide resolved
@hiddeco
Copy link
Member

hiddeco commented Jul 24, 2019

My vote goes to a fluxctl setup which outputs the YAMLs to stdout, based on a released version of our Helm chart. Re-using the Helm chart means we do not have to maintain multiple templates, and a released version because:

  1. The chart and the features it supports are on a different release schedule than fluxd and fluxctl releases. I think it is desirable to keep those on a separate release schedule, as this ensures complete freedom for both elements.
  2. We have been talking about moving chart/ to a dedicated repository, and to split the current chart into a fluxd and Helm operator chart, this would mean you can no longer embed from chart/flux/*.

@2opremio
Copy link
Contributor Author

Alright, will do. How about the command line options? Is that acceptable? I think linkerd does a more sophisticated thing, inspecting and exposing all the available chart options, but I think it's a bit overkill. I would rather keep it simple.

@hiddeco
Copy link
Member

hiddeco commented Jul 24, 2019

@2opremio the --extra-flux-args is going to be tricky, as the Helm chart offers an extraArgs key, but also has some defaults / dedicated keys for arguments that are missing in this command.

The Helm operator setting the default and a user supplying the argument here will have the desired effect (as Flux takes the value of the last supplied argument), but will not return a pretty YAML, which is probably a requirement.

@squaremo
Copy link
Member

Yep, simple for now!

On command-line options: those you have are a good base. You can remove --git-flux-subdir, --timeout, --amend since they don't pertain to generating a config.

Since people can mess around with the output as much as they want, it's not that important to get all the bells and whistles in, or to have an escape for extra args. But looking ahead a bit, I would like to see

  • --git-committer (or --git-user and --git-email), since that will soon become mandatory
  • --git-path (as a string slice), since I expect it to be used a lot

as arguments.

@squaremo
Copy link
Member

Versioning is tricky isn't it.

If you build in whatever's in the files (either the chart or example deployment) at the time of release, that's probably going to have most of the bells and whistles from fluxd in that release -- but it won't correspond to a released chart, which always comes after a fluxd release.

If you include the last released chart, in general it won't have arguments for fluxd in the same release.

If you try to use the latest chart, fluxctl has to understand, for all supported versions -- which will have to include future versions, since see above -- how to encode the arguments it's given into Helm values.


With those points in mind, here's an idea: make the deploy/ examples into very simple text templates, along with all the comments and commented-out bits, and bundle those into the fluxctl binary.

Pros:

  • will always (barring mistakes) be up-to-date with respect to the released fluxd
  • the output will retain all the useful comments and optional bits
  • easier to make it match the exact arguments supported by fluxctl setup

Cons:

  • a bit proprietary?

@2opremio
Copy link
Contributor Author

2opremio commented Jul 24, 2019

With those points in mind, here's an idea: make the deploy/ examples into very simple text templates, along with all the comments and commented-out bits, and bundle those into the fluxctl binary.

How about using Kustomize as a library and use generated-patches to modify the manifests on deploy/* on the fly? This will allow us leave the the manifest files unchanged and save us from maintaining separate templates (or invent an annotation language to maintain the existing templates)

EDIT: this won't preserve comments because Kustomize doesn't support it AFAIK

@2opremio
Copy link
Contributor Author

Or, we can create deploy-templates/ , a Go-templated version of deploy/ for the parameters used by fluxctl setup. We can embed those files in fluxctl and generate deploy/ with the existing values during the build.

@squaremo
Copy link
Member

squaremo commented Jul 24, 2019

How about using Kustomize as a library and use generated-patches to modify the manifests on deploy/* on the fly?

Pros:

  • not proprietary
  • serves secondary purpose as an example of how to configure flux using kustomize

Cons:

  • will wipe out the comments and commented-out sections, so the output won't be self-documenting (but we might be able to recover that just by supplying good examples/docs elsewhere)

@squaremo
Copy link
Member

Or, we can create deploy-templates/ , a Go-templated version of deploy/

I reckon (either for this or for the kustomize version) just replace it. It doesn't especially serve a purpose other than as a starting point, now -- you can't just apply what's in there, for instance.

@2opremio
Copy link
Contributor Author

serves secondary purpose as an example of how to configure flux using kustomize

My idea was to apply the patches on the fly and print the patches manifests.

@2opremio
Copy link
Contributor Author

2opremio commented Jul 24, 2019

It doesn't especially serve a purpose other than as a starting point, now -- you can't just apply what's in there, for instance.

I did that all the time when developing (after tweaking the git repo). Also, the guides rely on it.

@squaremo
Copy link
Member

squaremo commented Jul 24, 2019

I did that all the time when developing (after tweaking the parameters). Also, the guides rely on it.

Right, you had to tweak it, i.e., edit it. You would still be able to do that, especially if it's a kustomize input.

@2opremio
Copy link
Contributor Author

Ah, but you are saying there is no need for that after fluxctl install.

@2opremio 2opremio force-pushed the fluxctl-setup branch 2 times, most recently from 568ba2f to 09c421b Compare July 24, 2019 12:19
chart/flux/README.md Outdated Show resolved Hide resolved
@2opremio 2opremio changed the title [WIP] Add fluxctl setup command Add fluxctl install command Jul 24, 2019
@2opremio 2opremio marked this pull request as ready for review July 24, 2019 15:31
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

I would undo the changes to the helm chart and restore the deploy dir. If people use Kustomize they will want the YAMLs in git see #2253

@2opremio 2opremio force-pushed the fluxctl-setup branch 4 times, most recently from d3922e0 to c312d79 Compare July 25, 2019 14:30
deploy/flux-deployment.yaml Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the fluxctl-setup branch 3 times, most recently from 8a55252 to 909b9b0 Compare July 27, 2019 10:13
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Tidy and clean, thanks Fons 🌷

@2opremio 2opremio merged commit 077060e into fluxcd:master Jul 29, 2019
@2opremio 2opremio deleted the fluxctl-setup branch July 29, 2019 08:44
@squaremo
Copy link
Member

WIP: Add fluxctl setup

This could have done with a bit of a rebass before merging :-/ Never mind, done now.

2opremio added a commit to eksctl-io/eksctl that referenced this pull request Jul 29, 2019
now that fluxcd/flux#2287 is merged we can use
upstream flux
marccarre pushed a commit to eksctl-io/eksctl that referenced this pull request Jul 30, 2019
Now that fluxcd/flux#2287 is merged we can use
upstream Flux
marccarre pushed a commit to eksctl-io/eksctl that referenced this pull request Jul 31, 2019
Now that fluxcd/flux#2287 is merged we can use
upstream Flux
marccarre pushed a commit to eksctl-io/eksctl that referenced this pull request Aug 1, 2019
* Add `eksctl install flux` command

* Bump github/2opremio/flux

* Fix typo

* Refine missing pod error detection

* Avoid commit error when manifests already exist

* Fix missing parameter

* Address code review: flux -> Flux in CLI args' help

* Address code review: clearer call to action w.r.t. Flux SSH key

* Use upstream Flux version

Now that fluxcd/flux#2287 is merged we can use
upstream Flux

* gofmt -s -w pkg/ctl/install/install.go

* Address code review: use CreateOrUpdate instead of kubectl

* Address code review: improve use of RawExtensions and CreateOrUpdate

* Address code review: improved output of eksctl help install flux

* Address code review: rename import: k8serrors -> apierrors

... in order to follow the same conventions as elsewhere in the codebase.

* Address code review: add more context to comments?

* Address code review: clearer command description?

* Address code review: explain why we return Flux's SSH key

* Rename kubernetes.JoinManifest* to ConcatManifest*

... and also change ConcatManifests to become a variadic function, since more flexible to use.

* Add utilities to generate Kubernetes Namespace objects (+tests)

* Use kubernetes.Manifest{,YAML} utilities to install Flux

* Address code review: extract check of existence of namespace

+ update comments

* Address code review: re-read manifests once amended

* Create the Flux namespace in the same way as all other resources

* Fix rebase: GetCredentials -> RefreshClusterConfig

* Add pkg/kubernetes/client.go#RawClient#ClientOrUpdate higher-level method

This should hopefully make it easier to leverage RawClient for simpler cases.
@squaremo squaremo added this to the 1.14.0 milestone Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants