Navigation Menu

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

KIALI-686: Adding documentation to validation system #210

Merged
merged 1 commit into from May 28, 2018

Conversation

xeviknal
Copy link
Member

Thinking that we could also add somewhere the description of #180. Should we start a wiki?

@@ -92,6 +92,9 @@ func ServiceHealth(w http.ResponseWriter, r *http.Request) {
}

// ServiceIstioValidations is the API handler to get istio validations of a single service
// Input: namespace, service name
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the graph.go started to document the handlers.
I would use/follow same format in wording and also how to define input parameters and result, then for next handlers we can use it as template.
If not each handler will follow a different style (correct) but I guess we can unify here.

@@ -42,17 +47,20 @@ func (in RouteRuleChecker) runIndividualChecks() *models.IstioTypeValidations {
return &typeValidations
}

// Runs groupal checks for all route rules
Copy link
Contributor

Choose a reason for hiding this comment

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

groupal ? I guess that "Runs group checks for all route rules" could be enough.

@@ -74,6 +82,7 @@ func runChecks(routeRule kubernetes.IstioObject, typeValidations *models.IstioTy
checkersWg.Wait()
}

// Runs the specific checked passed by parameter and store its result into nameValidations under objectName map.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/checked/checker ?

@@ -10,6 +10,9 @@ import (

type PrecedenceChecker struct{ kubernetes.IstioObject }

// PrecedenceChecker checks if the current RouteRule has a valid value in precedence field.
// Output: It builds a IstioCheck whenever has an invalid value for precedence.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would try to follow same syntax used in others method. For example, instead of "Output:" in other method we are using "It returns".
Nothing wrong with "Output:" but just a suggestion to unify and have a close single style (when possible).

// 2. All weights have value between 0 and 100.
// 3. Sum of all weights are 100 (if only one weight, then it assumes that is 100).
// 4. All the route has to have weight label.
// Output: It builds a IstioCheck whenever has an invalid value for precedence.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here with the Output, I would try to follow same style used in other areas.

@@ -16,6 +16,8 @@ type ObjectChecker interface {
Check() *models.IstioTypeValidations
}

// Given a Namespace and service name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar here, "GetServiceValidations returns ..."

@@ -1,6 +1,6 @@
package models

// IstioTypeValidations represents a set of IstioNameValidations grouper per Istio ObjectType.
// IstioTypeValidations represents a set of IstioNameValidations grouped per Istio ObjectType.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@xeviknal
Copy link
Member Author

@lucasponce comments addressed 👍

@xeviknal
Copy link
Member Author

@lucasponce do you think that can get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants