From e85716d9bb2796d6a8704f06293e77117012f37b Mon Sep 17 00:00:00 2001 From: chamodshehanka Date: Sun, 6 Apr 2025 20:24:31 +0530 Subject: [PATCH 1/2] Fix: Typos and Erros and improve grammar in graph, metadata and runtime --- CONTRIBUTING.md | 10 +++---- pkg/graph/builder.go | 46 ++++++++++++++++++-------------- pkg/graph/graph.go | 5 ++-- pkg/graph/resource.go | 10 +++---- pkg/graph/validation.go | 2 +- pkg/graph/variable/variable.go | 10 +++---- pkg/metadata/owner_reference.go | 4 +-- pkg/metadata/selectors.go | 2 +- pkg/requeue/requeue.go | 6 ++--- pkg/runtime/interfaces.go | 6 ++--- pkg/runtime/resolver/resolver.go | 5 ++-- pkg/runtime/runtime.go | 14 +++++----- pkg/runtime/state.go | 10 +++---- 13 files changed, 69 insertions(+), 61 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 81e7d81f..08cbe2d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,6 +1,6 @@ # Contributing Guidelines -Thank you for your interest in contributing the project. Whether it's a bug report, new feature, correction, or additional documentation, we greatly value feedback and contributions from our community. +Thank you for your interest in contributing to the project. Whether it's a bug report, new feature, correction, or additional documentation, we greatly value feedback and contributions from our community. Please read through this document before submitting any issues or pull requests to ensure we have all the necessary information to effectively respond to your bug report or contribution. @@ -19,7 +19,7 @@ Please try to include detailed descriptions in issues. Details like these are in Note to keep things tidy and moving along, the maintainers may close issues that appear to be duplicates, are incomplete, or have no discussion for a period of time. -If an issue is closed and you feel like it needs to be open, we are always open to discussion. +If an issue is closed, and you feel like it needs to be open, we are always open to discussion. ## Contributing via Pull Requests @@ -47,12 +47,12 @@ GitHub provides nice documentation on [forking a repository](https://help.github As with issues above, to keep things tidy and moving along, the maintainers may close PRs that appear to be duplicates, are incomplete and have no discussion for a period of time, or that don't follow the process in spirit. -If a PR is closed and you feel like it needs to be open, we are always open to discussion. +If a PR is closed, and you feel like it needs to be open, we are always open to discussion. ## Development setup -Setup the [local enviroment](docs/developer-getting-started.md) to build and test the code locally. +Setup the [local environment](docs/developer-getting-started.md) to build and test the code locally. ## Finding contributions to work on Looking at the existing issues is a great way to find something to work on. As the project uses the default GitHub issue labels (enhancement/bug/duplicate/help wanted/invalid/question/wontfix), looking at any 'help wanted' issues is a great place to start. @@ -65,7 +65,7 @@ This project has adopted the [CNCF Code of Conduct](https://github.com/cncf/foun ## Security -If you discover a potential security issue in this project we ask that you notify project maintainers via email at security@kro.run. _Please do not create a public GitHub issue._ +If you discover a potential security issue in this project, we ask that you notify project maintainers via email at security@kro.run. _Please do not create a public GitHub issue._ ## Licensing diff --git a/pkg/graph/builder.go b/pkg/graph/builder.go index 2ed77168..4a54e819 100644 --- a/pkg/graph/builder.go +++ b/pkg/graph/builder.go @@ -60,8 +60,8 @@ func NewBuilder( return rgBuilder, nil } -// Builder is an object that is responsible of constructing and managing -// resourceGraphDefinitions. It is responsible of transforming the resourceGraphDefinition CRD +// Builder is an object that is responsible for constructing and managing +// resourceGraphDefinitions. It is responsible for transforming the resourceGraphDefinition CRD // into a runtime representation that can be used to create the resources in // the cluster. // @@ -78,7 +78,7 @@ func NewBuilder( // // If any of the above steps fail, the Builder will return an error. // -// The resulting ResourceGraphDefinition object is a fulyl processed and validated +// The resulting ResourceGraphDefinition object is a fully processed and validated // representation of a resource graph definition CR, it's underlying resources, and the // relationships between the resources. This object can be used to instantiate // a "runtime" data structure that can be used to create the resources in the @@ -208,8 +208,10 @@ func (b *Builder) NewResourceGraphDefinition(originalCR *v1alpha1.ResourceGraphD } // Before getting into the dependency graph, we need to validate the CEL expressions - // in the instance resource. In order to do that, we need to isolate each resource - // and evaluate the CEL expressions in the context of the resource graph definition. This is done + // in the instance resource. + // To do that, we need to isolate each resource + // and evaluate the CEL expressions in the context of the resource graph definition. + //This is done // by dry-running the CEL expressions against the emulated resources. err = validateResourceCELExpressions(resources, instance) if err != nil { @@ -220,7 +222,7 @@ func (b *Builder) NewResourceGraphDefinition(originalCR *v1alpha1.ResourceGraphD // building the resource graph definition. Understanding the relationships between the // resources in the resource graph definition a.k.a the dependency graph. // - // The dependency graph is an directed acyclic graph that represents the + // The dependency graph is a directed acyclic graph that represents the // relationships between the resources in the resource graph definition. The graph is // used to determine the order in which the resources should be created in the // cluster. @@ -307,7 +309,7 @@ func (b *Builder) buildRGResource(rgResource *v1alpha1.Resource, namespacedResou } for _, fieldDescriptor := range fieldDescriptors { resourceVariables = append(resourceVariables, &variable.ResourceField{ - // Assume variables are static, we'll validate them later + // Assume variables are static; we'll validate them later Kind: variable.ResourceVariableKindStatic, FieldDescriptor: fieldDescriptor, }) @@ -344,12 +346,16 @@ func (b *Builder) buildRGResource(rgResource *v1alpha1.Resource, namespacedResou } // buildDependencyGraph builds the dependency graph between the resources in the -// resource graph definition. The dependency graph is an directed acyclic graph that represents -// the relationships between the resources in the resource graph definition. The graph is used +// resource graph definition. +// The dependency graph is a directed acyclic graph that represents +// the relationships between the resources in the resource graph definition. +// The graph is used // to determine the order in which the resources should be created in the cluster. // -// This function returns the DAG, and a map of runtime variables per resource. later -// on we'll use this map to resolve the runtime variables. +// This function returns the DAG, and a map of runtime variables per resource. +// Later +// +// on, we'll use this map to resolve the runtime variables. func (b *Builder) buildDependencyGraph( resources map[string]*Resource, ) ( @@ -428,7 +434,7 @@ func (b *Builder) buildInstanceResource( // to request the creation of the resources defined in the resource graph definition. // // The instance resource is a Kubernetes resource, differently from typical - // CRDs, it doesn't have an OpenAPI schema. Instead, it has a schema defined + // CRDs; it doesn't have an OpenAPI schema. Instead, it has a schema defined // using the "SimpleSchema" format, a new standard we created to simplify // CRD declarations. @@ -486,7 +492,7 @@ func (b *Builder) buildInstanceResource( instanceStatusVariables := []*variable.ResourceField{} for _, statusVariable := range statusVariables { - // These variables needs to be injected into the status field of the instance. + // These variables need to be injected into the status field of the instance. path := "status." + statusVariable.Path statusVariable.Path = path @@ -565,7 +571,7 @@ func buildStatusSchema( // statusStructureParts := make([]schema.FieldDescriptor, 0, len(extracted)) statusDryRunResults := make(map[string][]ref.Val, len(fieldDescriptors)) for _, found := range fieldDescriptors { - // For each expression in the extracted ExpressionField we need to dry-run + // For each expression in the extracted `ExpressionField` we need to dry-run // the expression to infer the type of the status field. evals := []ref.Val{} for _, expr := range found.Expressions { @@ -615,8 +621,8 @@ func validateCELExpressionContext(env *cel.Env, expression string, resources []s } // dryRunExpression executes the given CEL expression in the context of a set -// of emulated resources. We could've called this function evaluateExpression -// but we chose to call it dryRunExpression to indicate that we are not actually +// of emulated resources. We could've called this function evaluateExpression, +// but we chose to call it dryRunExpression to indicate that we are not // used for anything other than validating the expression and inspecting it func dryRunExpression(env *cel.Env, expression string, resources map[string]*Resource) (ref.Val, error) { ast, issues := env.Compile(expression) @@ -645,7 +651,7 @@ func dryRunExpression(env *cel.Env, expression string, resources map[string]*Res } // extractDependencies extracts the dependencies from the given CEL expression. -// It returns a list of dependencies and a boolea indicating if the expression +// It returns a list of dependencies and a boolean indicating if the expression // is static or not. func extractDependencies(env *cel.Env, expression string, resourceNames []string) ([]string, bool, error) { // We also want to allow users to refer to the instance spec in their expressions. @@ -680,7 +686,7 @@ func extractDependencies(env *cel.Env, expression string, resourceNames []string // // In this process, we pin a resource and evaluate the CEL expressions in the // context of emulated resources. Meaning that given 3 resources A, B, and C, -// we evalute A's CEL expressions against 2 emulated resources B and C. Then +// we evaluate A's CEL expressions against 2 emulated resources B and C. Then // we evaluate B's CEL expressions against 2 emulated resources A and C, and so // on. func validateResourceCELExpressions(resources map[string]*Resource, instance *Resource) error { @@ -701,8 +707,8 @@ func validateResourceCELExpressions(resources map[string]*Resource, instance *Re // create includeWhenContext includeWhenContext := map[string]*Resource{} - // for now we will only support the instance context for includeWhen expressions. - // With this decision we will decide in creation time, and update time + // For now, we will only support the instance context for includeWhen expressions. + // With this decision, we will decide on creation time and update time // If we'll be creating resources or not includeWhenContext["schema"] = &Resource{ emulatedObject: &unstructured.Unstructured{ diff --git a/pkg/graph/graph.go b/pkg/graph/graph.go index d5047857..f40dc5d2 100644 --- a/pkg/graph/graph.go +++ b/pkg/graph/graph.go @@ -20,8 +20,9 @@ import ( "github.com/kro-run/kro/pkg/runtime" ) -// Graph represents a processed resourcegraphdefinition. It contains the DAG representation -// and everything needed to "manage" the resources defined in the resource graph definition. +// The Graph represents a processed resourcegraphdefinition. +// It contains the DAG representation and everything needed to "manage" +// the resources defined in the resource graph definition. type Graph struct { // DAG is the directed acyclic graph representation of the resource graph definition. DAG *dag.DirectedAcyclicGraph[string] diff --git a/pkg/graph/resource.go b/pkg/graph/resource.go index e2c6fb32..7d865d19 100644 --- a/pkg/graph/resource.go +++ b/pkg/graph/resource.go @@ -27,12 +27,12 @@ import ( // Resource represents a resource in a resource graph definition, it hholds // information about the resource, its schema, and its variables. // -// This object can only be created by the GraphBuilder and it should -// not be created manually. Also this object isn't designed to be +// This object can only be created by the GraphBuilder, and it should +// not be created manually. Also, this object isn't designed to be // modified after creation. type Resource struct { - // id is the unique identifier of the resource. It's the name of the - // resource in the resource graph definition. + // `id` is the unique identifier of the resource. + // It's the name of the resource in the resource graph definition. // An id is unique within a resource graph definition, and adheres to the naming // conventions. id string @@ -113,7 +113,7 @@ func (r *Resource) GetOrder() int { return r.order } -// GetGroupVersionKind returns the GVK of the resource. +// GetGroupVersionResource GetGroupVersionKind returns the GVK of the resource. func (r *Resource) GetGroupVersionResource() schema.GroupVersionResource { return r.gvr } diff --git a/pkg/graph/validation.go b/pkg/graph/validation.go index c67820fd..939fdcb0 100644 --- a/pkg/graph/validation.go +++ b/pkg/graph/validation.go @@ -104,7 +104,7 @@ func validateResourceGraphDefinitionNamingConventions(rgd *v1alpha1.ResourceGrap // The KRO naming convention is as follows: // - The id should start with a lowercase letter. // - The id should only contain alphanumeric characters. -// - does not contain any special characters, underscores, or hyphens. +// - Does not contain any special characters, underscores, or hyphens. func validateResourceIDs(rgd *v1alpha1.ResourceGraphDefinition) error { seen := make(map[string]struct{}) for _, res := range rgd.Spec.Resources { diff --git a/pkg/graph/variable/variable.go b/pkg/graph/variable/variable.go index 6ad772ec..d3ccb4b5 100644 --- a/pkg/graph/variable/variable.go +++ b/pkg/graph/variable/variable.go @@ -43,7 +43,7 @@ type FieldDescriptor struct { StandaloneExpression bool } -// ResourceVariable represents a variable in a resource. Variables are any +// ResourceField ResourceVariable represents a variable in a resource. Variables are any // field in a resource (under resources[*].definition) that is not a constant // value a.k.a contains one or multiple expressions. For example // @@ -55,10 +55,10 @@ type FieldDescriptor struct { // execution and their value is constant. Dynamic variables are resolved at // runtime and their value can change during the execution. // -// ResourceVariables are an extension of CELField and they contain additional +// ResourceVariables are an extension of CELField, and they contain additional // information about the variable kind. type ResourceField struct { - // CELField is the object that contains the expression, the path, and the + // CELField is the object that contains the expression, the path, and // the expected type (OpenAPI schema). FieldDescriptor // ResourceVariableKind is the kind of the variable (static or dynamic). @@ -81,13 +81,13 @@ func (rv *ResourceField) AddDependencies(dep ...string) { } } -// ResourceVariableKind represents the kind of a resource variable. +// ResourceVariableKind represents the kind of resource variable. type ResourceVariableKind string const ( // ResourceVariableKindStatic represents a static variable. Static variables // are resolved at the beginning of the execution and their value is constant. - // Static variables are easy to find, they always start with 'spec'. Refereing + // Static variables are easy to find, they always start with 'spec'. Referring // to the instance spec. // // For example: diff --git a/pkg/metadata/owner_reference.go b/pkg/metadata/owner_reference.go index 038ddd0c..6255c7fb 100644 --- a/pkg/metadata/owner_reference.go +++ b/pkg/metadata/owner_reference.go @@ -26,7 +26,7 @@ var ( KRORGOwnerReferenceAPIVersion = v1alpha1.GroupVersion.String() ) -// stamped on the CRD and RGIs +// NewResourceGraphDefinitionOwnerReference stamped on the CRD and RGIs func NewResourceGraphDefinitionOwnerReference(name string, uid types.UID) metav1.OwnerReference { return metav1.OwnerReference{ Name: name, @@ -37,7 +37,7 @@ func NewResourceGraphDefinitionOwnerReference(name string, uid types.UID) metav1 } } -// stamped on the RGI child resources +// NewInstanceOwnerReference stamped on the RGI child resources func NewInstanceOwnerReference(gvk schema.GroupVersionKind, name string, uid types.UID) metav1.OwnerReference { return metav1.OwnerReference{ Name: name, diff --git a/pkg/metadata/selectors.go b/pkg/metadata/selectors.go index 0142ebf9..5c991e0c 100644 --- a/pkg/metadata/selectors.go +++ b/pkg/metadata/selectors.go @@ -33,7 +33,7 @@ func NewResourceGraphDefinitionSelector(resourceGraphDefinition metav1.Object) m return metav1.LabelSelector{ MatchLabels: map[string]string{ ResourceGraphDefinitionIDLabel: string(resourceGraphDefinition.GetUID()), - // ResourceGraphDefinitionNameLabel: resourceGraphDefinition.GetName(), + // ResourceGraphDefinitionNameLabel: resourceGraphDefinition.GetName(), // ResourceGraphDefinitionNamespaceLabel: resourceGraphDefinition.GetNamespace(), }, } diff --git a/pkg/requeue/requeue.go b/pkg/requeue/requeue.go index 53c1246a..8dd70845 100644 --- a/pkg/requeue/requeue.go +++ b/pkg/requeue/requeue.go @@ -103,9 +103,9 @@ func (e *RequeueNeeded) Unwrap() error { var _ error = &RequeueNeeded{} // RequeueNeededAfter instructs the ACK runtime to requeue the processing item -// after specified duration without been logged as error. This should be used -// when a "error condition" occurrence is sort of expected and can be resolved -// by retry. e.g. a dependency haven't been fulfilled yet, and expected it to +// after specified duration without been logged as error. This should be used +// when an "error condition" occurrence is sort of expected and can be resolved +// by retry. E.g., a dependency hasn't been fulfilled yet, and expected it to // be fulfilled after duration. Note: use this with care, a simple wait might // suit your use case better. type RequeueNeededAfter struct { diff --git a/pkg/runtime/interfaces.go b/pkg/runtime/interfaces.go index 6c834725..90ee43ad 100644 --- a/pkg/runtime/interfaces.go +++ b/pkg/runtime/interfaces.go @@ -80,13 +80,13 @@ type Interface interface { // 1. The runtime package depends on how resources are defined in the graph // package. // -// 2. the graph package needs to instantiate the a runtime instance to during +// 2. The graph package needs to instantiate a runtime instance during // the reconciliation process. // -// 3. the graph package needs to classify the variables and dependencies of +// 3. The graph package needs to classify the variables and dependencies of // a resource to build the graph. The runtime package needs to know about // these variables and dependencies to resolve the resources. -// This utility is moved to the types package. (Thinking about moving it +// This utility is moved to the `types` package. (Thinking about moving it // to a new package called "internal/typesystem/variables") type ResourceDescriptor interface { // GetGroupVersionResource returns the k8s GVR for this resource. Note that diff --git a/pkg/runtime/resolver/resolver.go b/pkg/runtime/resolver/resolver.go index 5df69f09..154f14df 100644 --- a/pkg/runtime/resolver/resolver.go +++ b/pkg/runtime/resolver/resolver.go @@ -95,8 +95,9 @@ func (r *Resolver) resolveField(field variable.FieldDescriptor) ResolutionResult value, err := r.getValueFromPath(field.Path) if err != nil { - // Not sure if these kind of errors should be fatal, these paths are produced - // by the parser, so they should be valid. Maybe we should log them instead.... + // Not sure if these kinds of errors should be fatal, these paths are produced + // by the parser, so they should be valid. + // Maybe we should log them instead… result.Error = fmt.Errorf("error getting value: %v", err) return result } diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 6a349572..695edd0b 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -61,10 +61,10 @@ func NewResourceGraphDefinitionRuntime( // Process the resource variables. for _, variable := range resource.GetVariables() { for _, expr := range variable.Expressions { - // If cached use the same pointer. + // If cached, use the same pointer. if ec, seen := r.expressionsCache[expr]; seen { // NOTE(a-hilaly): This strikes me as an early optimization, but - // it's a good one, i believe... We can always remove it if it's + // it's a good one, I believe... We can always remove it if it's // too magical. r.runtimeVariables[id] = append(r.runtimeVariables[id], ec) continue @@ -163,7 +163,7 @@ type ResourceGraphDefinitionRuntime struct { // synchronization. topologicalOrder []string - // ignoredByConditionsResources holds the resources whos defined conditions returned false + // ignoredByConditionsResources holds the resources who's defined conditions returned false // or who's dependencies are ignored ignoredByConditionsResources map[string]bool } @@ -352,7 +352,7 @@ func (rt *ResourceGraphDefinitionRuntime) evaluateDynamicVariables() error { return err } - // let's iterate over any resolved resource and try to resolve + // Let's iterate over any resolved resource and try to resolve // the dynamic variables that depend on it. // Since we have already cached the expressions, we don't need to // loop over all the resources. @@ -498,7 +498,7 @@ func (rt *ResourceGraphDefinitionRuntime) IsResourceReady(resourceID string) (bo return true, "", nil } -// IgnoreResource ignores resource that has a conditions expressison that evaluated +// IgnoreResource ignores resource that has a condition expression that evaluated // to false or whose dependencies are ignored func (rt *ResourceGraphDefinitionRuntime) IgnoreResource(resourceID string) { rt.ignoredByConditionsResources[resourceID] = true @@ -550,7 +550,7 @@ func (rt *ResourceGraphDefinitionRuntime) WantToCreateResource(resourceID string } // returning a reason here to point out which expression is not ready yet if !value.(bool) { - return false, fmt.Errorf("Skipping resource creation due to condition %s", condition) + return false, fmt.Errorf("skipping resource creation due to condition %s", condition) } } return true, nil @@ -569,7 +569,7 @@ func evaluateExpression(env *cel.Env, context map[string]interface{}, expression } // We get an error here when the value field we're looking for is not yet defined // For now leaving it as error, in the future when we see different scenarios - // of this error we can make some a reason, and others an error + // of this error, we can make some a reason, and others an error val, _, err := program.Eval(context) if err != nil { return nil, fmt.Errorf("failed evaluating expression %s: %w", expression, err) diff --git a/pkg/runtime/state.go b/pkg/runtime/state.go index b333cc5b..381d3e5c 100644 --- a/pkg/runtime/state.go +++ b/pkg/runtime/state.go @@ -36,15 +36,15 @@ const ( // ResourceStateWaitingOnReadiness indicates that the resource is waiting // for its readiness conditions to be met. This typically occurs after the - // resource has been created or updated, but is not yet in a stable state - // according to its defined readiness criteria. e.g waiting for a Pod to + // resource has been created or updated, but is not yet in a stable state, + // according to its defined readiness criteria. E.g., waiting for a Pod to // be running and ready, a PVC to be bound ... ResourceStateWaitingOnReadiness ResourceState = "WaitingOnReadiness" // ResourceStateIgnoredByConditions indicates that the resource is ignored // by a condition that evaluated to false. This typically occurs before // a resource is created or updated, and is decided by a variable defined - // in the instance spec. Eg. Deciding whether to create a Deployment or + // in the instance spec. E.g., Deciding whether to create a Deployment or // just a simple pod based on the defined replica. ResourceStateIgnoredByConditions ResourceState = "IgnoredByConditions" ) @@ -71,8 +71,8 @@ type expressionEvaluationState struct { // dynamic. This affects when and how the expression is evaluated. Kind variable.ResourceVariableKind - // Resolved indicates whether the expression has been successfully - // evaluated. Its set to true once the expression is evaluated without + // Resolved indicates whether the expression has been successfully evaluated. + // It's set to true once the expression is evaluated without // errors and a value is obtained. Resolved bool From 984c36a216291a6ee117bc0ad8f9a829b72d5eab Mon Sep 17 00:00:00 2001 From: chamodshehanka Date: Tue, 8 Apr 2025 23:33:18 +0530 Subject: [PATCH 2/2] chore: unit testing fixed --- pkg/runtime/runtime_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/runtime/runtime_test.go b/pkg/runtime/runtime_test.go index bc357344..9942eb5f 100644 --- a/pkg/runtime/runtime_test.go +++ b/pkg/runtime/runtime_test.go @@ -2376,7 +2376,7 @@ func Test_WantToCreateResource(t *testing.T) { return } if tt.wantSkip { - if err == nil || !strings.Contains(err.Error(), "Skipping resource creation due to condition") { + if err == nil || !strings.Contains(err.Error(), "skipping resource creation due to condition") { t.Errorf("WantToCreateResource() expected skip message, got %v", err) } return