Skip to content

Improve gramar and fix errors messages in graph, metadata and runtime package #485

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

Merged
merged 2 commits into from
Apr 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -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.

Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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 [email protected]. _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 [email protected]. _Please do not create a public GitHub issue._

## Licensing

Expand Down
46 changes: 26 additions & 20 deletions pkg/graph/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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,
})
Expand Down Expand Up @@ -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,
) (
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down
5 changes: 3 additions & 2 deletions pkg/graph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
10 changes: 5 additions & 5 deletions pkg/graph/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/graph/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 5 additions & 5 deletions pkg/graph/variable/variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand All @@ -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).
Expand All @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions pkg/metadata/owner_reference.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pkg/metadata/selectors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/requeue/requeue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions pkg/runtime/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions pkg/runtime/resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading
Loading