Skip to content
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 components verification step to bootstraping WGE #3675

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Samra10
Copy link
Member

@Samra10 Samra10 commented Nov 27, 2023

Closes #3674

What changed?

  • Install_WGE step updated to check different components' health before proceeding to the next step.

Why was this change made?

  • To verify that the WGE installation is healthy before installing additional components.

How was this change implemented?

  • Add a new file under bootstrap utils for checking status.
  • use the status methods to check WGE components

How did you validate the change?

  • Explain how a reviewer can verify the change themselves

  • Using interactive or non-interactive WGE bootstrapping, after the WGE helmRepo gets rendered the health checks will start to verify the health for multiple deployments. once all the components predefined on a list are ready the health check will pass, if not all desired components not healthy after a predefined time-out it will exist.

  • Unit tests -- what is covered, what cannot be covered; are
    there tests that fail without the change?

  • Components verification can't be covered on the unit-test

Release notes

Documentation Changes

Other follow ups

@Samra10 Samra10 added enhancement New feature or request area/cli issues related to gitops ee cli labels Nov 27, 2023
@@ -39,6 +41,10 @@ const (
gitopssetsHealthBindAddress = ":8081"
)

var Components = []string{"cluster-controller-manager",
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the suggested components to be checked, do u think we should add more?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that we should try to see whether the list could be dynamic over static

Copy link
Contributor

Choose a reason for hiding this comment

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

for the installwge step, the components are being defined as part of the helm chart and its dependencies

https://github.com/weaveworks/weave-gitops-enterprise/blob/main/charts/mccp/Chart.yaml

an strategy to resolve the components could be created out of it

@Samra10 Samra10 marked this pull request as ready for review November 30, 2023 01:13
Copy link
Contributor

@enekofb enekofb left a comment

Choose a reason for hiding this comment

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

Please, could you share in the PR description what are the testing scenarios that you have used and explain how could i verify the behavior?

Given the components that you want to asses health are deployed after the call to the function

https://github.com/weaveworks/weave-gitops-enterprise/pull/3675/files#diff-c8e6ae555f83215266745d9cdffeaed0e7170957687b6d10b2026465fc44becdR165-R167

I might think that are not been checked in the right moment of the flow.

@Samra10
Copy link
Member Author

Samra10 commented Dec 3, 2023

I might think that are not been checked in the right moment of the flow.

  • For now I'm verifying it after the helmRealese created and pods start to be deployed, do u think there is a better place?

  • And should we add the same step after any extra components created or modified for example OIDC and the extra components?

@enekofb
Copy link
Contributor

enekofb commented Dec 4, 2023

I might think that are not been checked in the right moment of the flow.

  • For now I'm verifying it after the helmRealese created and pods start to be deployed, do u think there is a better place?
    umms, lets check in our sync, i might be missing something in the PR

@Samra10 Samra10 requested a review from enekofb December 6, 2023 21:08
Name string
Input []StepInput
Step func(input []StepInput, c *Config) ([]StepOutput, error)
Verify func(output []StepOutput, c *Config) error
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

please add the internal documentation on guidance to use it

Copy link
Contributor

@enekofb enekofb left a comment

Choose a reason for hiding this comment

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

great progress @Samra10 please see my suggestions which are in the direction of:

  • trying to have a dynamic set of components over static
  • add unit testing
  • complete code documentation and internal docs

verified the happy and unhappy paths 👌

Name string
Input []StepInput
Step func(input []StepInput, c *Config) ([]StepOutput, error)
Verify func(output []StepOutput, c *Config) error
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the internal documentation on guidance to use it

@@ -148,6 +148,8 @@ func TestInstallWge_Execute(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
step := NewInstallWGEStep()
// skip verify step
step.Verify = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess that missing testing here

Copy link
Contributor

Choose a reason for hiding this comment

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

how to unit test this logic ... possible paths

a) mock the behaviour via interface
b) given this is based on kstatus, see how to unit test kstatus https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md

logger logger.Logger
}

func NewStatusChecker(client k8s_client.Client, pollInterval time.Duration, timeout time.Duration, log logger.Logger) (*StatusChecker, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comments

"sigs.k8s.io/cli-utils/pkg/object"
)

type StatusChecker struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comments as exported

}, nil
}

func (sc *StatusChecker) Assess(identifiers ...object.ObjMetadata) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comments as exported

@@ -39,6 +41,10 @@ const (
gitopssetsHealthBindAddress = ":8081"
)

var Components = []string{"cluster-controller-manager",
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that we should try to see whether the list could be dynamic over static

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli issues related to gitops ee cli enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding CLI | Add verify step to ensure that the cli is in a consistent state
2 participants