-
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 components verification step to bootstraping WGE #3675
base: main
Are you sure you want to change the base?
Conversation
@@ -39,6 +41,10 @@ const ( | |||
gitopssetsHealthBindAddress = ":8081" | |||
) | |||
|
|||
var Components = []string{"cluster-controller-manager", |
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.
These are the suggested components to be checked, do u think we should add 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.
I guess that we should try to see whether the list could be dynamic over static
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.
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
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.
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
I might think that are not been checked in the right moment of the flow.
|
|
Name string | ||
Input []StepInput | ||
Step func(input []StepInput, c *Config) ([]StepOutput, error) | ||
Verify func(output []StepOutput, c *Config) error |
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!
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.
please add the internal documentation on guidance to use it
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.
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 |
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.
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 |
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 guess that missing testing here
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.
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) { |
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.
missing comments
"sigs.k8s.io/cli-utils/pkg/object" | ||
) | ||
|
||
type StatusChecker struct { |
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.
missing comments as exported
}, nil | ||
} | ||
|
||
func (sc *StatusChecker) Assess(identifiers ...object.ObjMetadata) error { |
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.
missing comments as exported
@@ -39,6 +41,10 @@ const ( | |||
gitopssetsHealthBindAddress = ":8081" | |||
) | |||
|
|||
var Components = []string{"cluster-controller-manager", |
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 guess that we should try to see whether the list could be dynamic over static
Closes #3674
What changed?
Why was this change made?
How was this change implemented?
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