-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add bootstrap CLI controllers configurations #3647
Conversation
a049adf
to
61e5aa5
Compare
61e5aa5
to
2409dc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope of this review covers the general code aspects of the PR. Raising it now so you could start having a look to the suggestions.
Next review will cover the extra controllers logic and one of the controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this review includes extra controllers logic and policy agent controller (as example of controller, my comments for policy-agent applies to the other controllers too)
839015c
to
507dabe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-ups / suggestions:
- Log known-issue for cert-manager in release notes and documentation
- handle cert-manager dependency
- to unify install controller logic into a single one with configuration if they are common
- (general) to have a better approach for cmd Examples: orient only to journeys, currently stands a bit weird as configuration options.
@@ -30,12 +31,16 @@ func askBootstrapFlux(input []StepInput, c *Config) ([]StepOutput, error) { | |||
if !canAskForFluxBootstrap(input, c) { | |||
return []StepOutput{}, nil | |||
} | |||
if c.BootstrapFlux && c.Silent { | |||
c.Logger.Generatef("bootstrapping flux in the generic way") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this message might be misleading as you are generating output ... it feels a better place where the action really happens
Name: inComponentsExtra, | ||
Type: multiSelectionChoice, | ||
Msg: componentsExtraMsg, | ||
Values: ComponentsExtra, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you should not be able to select all values but the one that you could install which are the total components - the ones that you have installed ... otherwise you could select to install tf-controller when it is already installed.
return []StepOutput{}, fmt.Errorf("can't install tf controller: %v", err) | ||
} | ||
case "": | ||
c.Logger.Successf("selected none extra components") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.Logger.Successf("selected none extra components") |
.. given there is nothing to do
@@ -204,6 +222,11 @@ func (cb *ConfigBuilder) Build() (Config, error) { | |||
return Config{}, fmt.Errorf("error creating git repository configuration: %v", err) | |||
} | |||
|
|||
componentsExtraConfig, err := NewInstallExtraComponentsConfig(cb.componentsExtra, kubeHttp.Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice!
policyAgentInstallConfirmMsg = "Policy Agent is installed successfully" | ||
) | ||
|
||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏆
pkg/bootstrap/steps/step.go
Outdated
@@ -147,9 +147,15 @@ func defaultInputStep(inputs []StepInput, c *Config, stdin io.ReadCloser) ([]Ste | |||
if input.StepInformation != "" { | |||
c.Logger.Warningf(input.StepInformation) | |||
} | |||
// if silent mode is enabled, select yes | |||
// if silent mode is enabled, select the default value | |||
// if no default value select yes by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// if no default value select yes by default | |
// if no default value an error will be returned |
"--password=admin123", | ||
"--discovery-url=https://dex-01.wge.dev.weave.works/.well-known/openid-configuration", | ||
"--client-id=weave-gitops-enterprise", | ||
gitUsernameFlag, gitPasswordFlag, gitBranchFlag, gitRepoPathFlag, | ||
repoHTTPSURLFlag, | ||
oidcClientSecretFlag, "-s", | ||
"--components-extra=policy-agent,tf-controller", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested that acceptance test passed
◎ reconciling changes
=== RUN TestBootstrapCmd/journey_flux_does_not_exist:_should_bootstrap_with_valid_arguments
✔ changes are reconciled successfully!
◎ install extra components
⚠️ please note that the Policy Agent requires cert-manager to be installed!
► installing Policy Agent ...
✔ Policy Agent is installed successfully
► write file to repo: policy-agent-helmrelease.yaml
► cloning flux git repo: flux-system/flux-system
✔ cloned flux git repo: flux-system/flux-system
✔ file committed to repo: policy-agent-helmrelease.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
► installing Terraform Controller ...
► rendered WGE HelmRelease file
► updating HelmRelease file
✔ Terraform Controller is installed successfully
► write file to repo: tf-controller.yaml
► cloning flux git repo: flux-system/flux-system
✔ cloned flux git repo: flux-system/flux-system
✔ file committed to repo: tf-controller.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
► write file to repo: wge-hrelease.yaml
► cloning flux git repo: flux-system/flux-system
✔ cloned flux git repo: flux-system/flux-system
✔ file committed to repo: wge-hrelease.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
◎ preparing dashboard domain
✔ WGE v0.37.0 is installed successfully. To access the dashboard, run the following command to create portforward to the dasboard local domain http://localhost:8000
► credentials for accessing the admin dashboard username: `wego-admin`
kubectl -n flux-system port-forward svc/clusters-service 8000:8000
cmd_acceptance_test.go:291: "level"=0 "msg"="deleted object" "name"="weave-gitops-enterprise-credentials" "ns"="flux-system" "kind"="Secret"
cmd_acceptance_test.go:291: "level"=0 "msg"="deleted object" "name"="cluster-user-auth" "ns"="flux-system" "kind"="Secret"
--- PASS: TestBootstrapCmd (130.15s)
--- PASS: TestBootstrapCmd/journey_flux_does_not_exist:_should_bootstrap_with_valid_arguments (130.14s)
"--password=admin123", | ||
"--discovery-url=https://dex-01.wge.dev.weave.works/.well-known/openid-configuration", | ||
"--client-id=weave-gitops-enterprise", | ||
gitUsernameFlag, gitPasswordFlag, gitBranchFlag, gitRepoPathFlag, | ||
repoHTTPSURLFlag, | ||
oidcClientSecretFlag, "-s", | ||
"--components-extra=policy-agent,tf-controller", | ||
"--bootstrap-flux", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could see the following pods
➜ weave-gitops-enterprise git:(3276-cli-controllers-eneko) ✗ k get pods <aws:sts>
NAME READY STATUS RESTARTS AGE
cluster-bootstrap-controller-manager-79cbb5dfbc-dz5rd 2/2 Running 0 76s
cluster-controller-manager-86c496b9f7-5tk4k 2/2 Running 0 76s
gitopssets-controller-manager-5cc97766b9-knvb4 2/2 Running 0 76s
helm-controller-b957fcf89-9ptqk 1/1 Running 0 2m2s
kustomize-controller-644f79985c-tqbdp 1/1 Running 0 2m2s
notification-controller-ddf44665d-r6q8f 1/1 Running 0 2m2s
source-controller-9dfbc5cd-cbs9x 1/1 Running 0 2m2s
tf-controller-7975f69b86-psdmx 1/1 Running 0 58s
tf-controller-7975f69b86-s9pm5 1/1 Running 0 58s
tf-controller-7975f69b86-w5bmt 1/1 Running 0 58s
weave-gitops-enterprise-mccp-cluster-service-756cc58859-kqt45 1/1 Running 0 20s
weave-gitops-enterprise-pipeline-controller-85bf5c67d6-459km 1/1 Running 0 76s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested invalid path
--repo-path=clusters/management --components-extra=tf-controller2
► creating client to cluster
✔ created client to cluster: https://127.0.0.1:51388
Error: cannot config bootstrap: cannot create components extra configuration: unsupported component selected: tf-controller2
exit status 1
} | ||
|
||
func installExtraComponents(input []StepInput, c *Config) ([]StepOutput, error) { | ||
for _, param := range input { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expectations:
- I should be able to skip installing
- i should be able to select one
- i should be able to select more than one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pr #3701
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
Here the follow-ups to consider from the previous review #3647 (review)
Closes #3276, #3465
What changed?
How did you validate the change?
a) using non-interactive
1- install with one controller
2- install with multiple controllers
gitops bootstrap --components-extra="policy-agent,tf-controller"
b) interactive session
run
gitops bootstrap
and follow on screen instructionsc) silent mode:
gitops bootstrap -s
Integration tests -- what is covered, what cannot be covered;
or, explain why there are no new tests
Unit tests -- what is covered, what cannot be covered; are
there tests that fail without the change?
Release notes
Documentation Changes
Other follow ups