Skip to content

Commit

Permalink
Make reconcilers work with the auto-converted struct ⛈
Browse files Browse the repository at this point in the history
With this commit, the reconcilers (PipelineRun and TaskRun) now uses
the v1alpha2 part of the struct instead of the old one. In a gist,
this means

- `TaskRun.Inputs.Params` becomes `TaskRun.Params`
- `TaskRun.Inputs.Resources` becomes `TaskRun.Resources.Inputs`
- `TaskRun.Outputs.Resources` becomes `TaskRun.Resources.Outputs`

Kubernetes can only store one version of the CRD, and before 1.17
does not support conversion webhook per-se. *Note* that once we have
conversion webhook it will be simpler ; support for conversion webhook
should land soon-ish in knative.dev/pkg which will allow us to easily
switch to it.

> 1. Pick a conversion strategy. Since custom resource objects need to
> be able to be served at both versions, that means they will
> sometimes be served at a different version than their storage
> version. In order for this to be possible, the custom resource
> objects must sometimes be converted between the version they are
> stored at and the version they are served at. If the conversion
> involves schema changes and requires custom logic, a conversion
> webhook should be used. If there are no schema changes, the default
> None conversion strategy may be used and only the apiVersion field
> will be modified when serving different versions.
> 2. If using conversion webhooks, create and deploy the conversion
> webhook. See the Webhook conversion for more details.

As written above, we can only store one version on the CRD and we want
to serve multilple. To keep the latest API the cleanest (here
`v1alpha2`), we are using `v1alpha1` and making sure that `v1alpha1`
struct are compatible (aka a `v1alpha2` struct can be serialized in a
`v1alpha1` struct so that it can be stored). This is what the
auto-conversion role is :

- take a version and convert it to the most recent version (here
  `v1alpha2`)
- then store it as the lowest version (here `v1alpha1`)

For the controller this means we still work with `v1alpha1`
struct *but* with only the part that is compatible with
`v1alpha2`.

One follow-up will be to rename the `v1alpha1` field that are not used
anymore, prepending them with `Deprecated`.

Signed-off-by: Vincent Demeester <[email protected]>
  • Loading branch information
vdemeester committed Feb 13, 2020
1 parent 0cfe1ea commit 3874ceb
Show file tree
Hide file tree
Showing 37 changed files with 1,156 additions and 739 deletions.
22 changes: 4 additions & 18 deletions pkg/apis/pipeline/v1alpha1/conversion_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,35 +17,21 @@ limitations under the License.
package v1alpha1

import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha2"
"knative.dev/pkg/apis"
)

const (
// ConditionTypeConvertible is a Warning condition that is set on
// resources when they cannot be converted to warn of a forthcoming
// breakage.
ConditionTypeConvertible apis.ConditionType = "Convertible"
ConditionTypeConvertible apis.ConditionType = v1alpha2.ConditionTypeConvertible
)

// CannotConvertError is returned when a field cannot be converted.
type CannotConvertError struct {
Message string
Field string
}
type CannotConvertError = v1alpha2.CannotConvertError

var _ error = (*CannotConvertError)(nil)

// Error implements error
func (cce *CannotConvertError) Error() string {
return cce.Message
}

// ConvertErrorf creates a CannotConvertError from the field name and format string.
func ConvertErrorf(field, msg string, args ...interface{}) error {
return &CannotConvertError{
Message: fmt.Sprintf(msg, args...),
Field: field,
}
}
var ConvertErrorf = v1alpha2.ConvertErrorf
14 changes: 14 additions & 0 deletions pkg/apis/pipeline/v1alpha1/task_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package v1alpha1
import (
"context"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha2"
"github.com/tektoncd/pipeline/pkg/contexts"
"knative.dev/pkg/apis"
)

Expand All @@ -30,6 +32,18 @@ func (t *Task) SetDefaults(ctx context.Context) {

// SetDefaults set any defaults for the task spec
func (ts *TaskSpec) SetDefaults(ctx context.Context) {
if contexts.IsUpgradeViaDefaulting(ctx) {
v := v1alpha2.TaskSpec{}
if ts.ConvertUp(ctx, &v) == nil {
alpha := TaskSpec{}
if alpha.ConvertDown(ctx, &v) == nil {
*ts = alpha
}
}
}
for i := range ts.Params {
ts.Params[i].SetDefaults(ctx)
}
if ts.Inputs != nil {
ts.Inputs.SetDefaults(ctx)
}
Expand Down
4 changes: 1 addition & 3 deletions pkg/apis/pipeline/v1alpha1/task_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,7 @@ type Inputs struct {
// the Task definition, and when provided as an Input, the Name will be the
// path to the volume mounted containing this Resource as an input (e.g.
// an input Resource named `workspace` will be mounted at `/workspace`).
type TaskResource struct {
ResourceDeclaration `json:",inline"`
}
type TaskResource = v1alpha2.TaskResource

// Outputs allow a task to declare what data the Build/Task will be producing,
// i.e. results such as logs and artifacts such as images.
Expand Down
47 changes: 42 additions & 5 deletions pkg/apis/pipeline/v1alpha1/task_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"path/filepath"
"strings"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha2"
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/substitution"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -79,9 +80,18 @@ func (ts *TaskSpec) Validate(ctx context.Context) *apis.FieldError {
}
}

// Validate Resources declaration
if err := ts.Resources.Validate(ctx); err != nil {
return err
}
// Validate that the parameters type are correct
if err := v1alpha2.ValidateParameterTypes(ts.Params); err != nil {
return err
}

// A task doesn't have to have inputs or outputs, but if it does they must be valid.
// A task can't duplicate input or output names.

// Deprecated
if ts.Inputs != nil {
for _, resource := range ts.Inputs.Resources {
if err := validateResourceType(resource, fmt.Sprintf("taskspec.Inputs.Resources.%s.Type", resource.Name)); err != nil {
Expand All @@ -95,6 +105,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) *apis.FieldError {
return err
}
}
// Deprecated
if ts.Outputs != nil {
for _, resource := range ts.Outputs.Resources {
if err := validateResourceType(resource, fmt.Sprintf("taskspec.Outputs.Resources.%s.Type", resource.Name)); err != nil {
Expand All @@ -117,10 +128,19 @@ func (ts *TaskSpec) Validate(ctx context.Context) *apis.FieldError {
}
}

if err := validateInputParameterVariables(ts.Steps, ts.Inputs); err != nil {
if err := v1alpha2.ValidateParameterVariables(ts.Steps, ts.Params); err != nil {
return err
}
// Deprecated
if err := validateInputParameterVariables(ts.Steps, ts.Inputs, ts.Params); err != nil {
return err
}

if err := v1alpha2.ValidateResourcesVariables(ts.Steps, ts.Resources); err != nil {
return err
}
if err := validateResourceVariables(ts.Steps, ts.Inputs, ts.Outputs); err != nil {
// Deprecated
if err := validateResourceVariables(ts.Steps, ts.Inputs, ts.Outputs, ts.Resources); err != nil {
return err
}
return nil
Expand Down Expand Up @@ -251,10 +271,17 @@ func validateInputParameterTypes(inputs *Inputs) *apis.FieldError {
return nil
}

func validateInputParameterVariables(steps []Step, inputs *Inputs) *apis.FieldError {
func validateInputParameterVariables(steps []Step, inputs *Inputs, params []v1alpha2.ParamSpec) *apis.FieldError {
parameterNames := map[string]struct{}{}
arrayParameterNames := map[string]struct{}{}

for _, p := range params {
parameterNames[p.Name] = struct{}{}
if p.Type == ParamTypeArray {
arrayParameterNames[p.Name] = struct{}{}
}
}
// Deprecated
if inputs != nil {
for _, p := range inputs.Params {
parameterNames[p.Name] = struct{}{}
Expand All @@ -270,13 +297,23 @@ func validateInputParameterVariables(steps []Step, inputs *Inputs) *apis.FieldEr
return validateArrayUsage(steps, "params", arrayParameterNames)
}

func validateResourceVariables(steps []Step, inputs *Inputs, outputs *Outputs) *apis.FieldError {
func validateResourceVariables(steps []Step, inputs *Inputs, outputs *Outputs, resources *v1alpha2.TaskResources) *apis.FieldError {
resourceNames := map[string]struct{}{}
if resources != nil {
for _, r := range resources.Inputs {
resourceNames[r.Name] = struct{}{}
}
for _, r := range resources.Outputs {
resourceNames[r.Name] = struct{}{}
}
}
// Deprecated
if inputs != nil {
for _, r := range inputs.Resources {
resourceNames[r.Name] = struct{}{}
}
}
// Deprecated
if outputs != nil {
for _, r := range outputs.Resources {
resourceNames[r.Name] = struct{}{}
Expand Down
14 changes: 13 additions & 1 deletion pkg/apis/pipeline/v1alpha1/taskrun_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"time"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha2"
"github.com/tektoncd/pipeline/pkg/contexts"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
Expand All @@ -31,7 +32,8 @@ var _ apis.Defaultable = (*TaskRun)(nil)
const managedByLabelKey = "app.kubernetes.io/managed-by"

func (tr *TaskRun) SetDefaults(ctx context.Context) {
tr.Spec.SetDefaults(ctx)
ctx = apis.WithinParent(ctx, tr.ObjectMeta)
tr.Spec.SetDefaults(apis.WithinSpec(ctx))

// If the TaskRun doesn't have a managed-by label, apply the default
// specified in the config.
Expand All @@ -45,6 +47,16 @@ func (tr *TaskRun) SetDefaults(ctx context.Context) {
}

func (trs *TaskRunSpec) SetDefaults(ctx context.Context) {
if contexts.IsUpgradeViaDefaulting(ctx) {
v := v1alpha2.TaskRunSpec{}
if trs.ConvertUp(ctx, &v) == nil {
alpha := TaskRunSpec{}
if alpha.ConvertDown(ctx, &v) == nil {
*trs = alpha
}
}
}

cfg := config.FromContextOrDefaults(ctx)
if trs.TaskRef != nil && trs.TaskRef.Kind == "" {
trs.TaskRef.Kind = NamespacedTaskKind
Expand Down
9 changes: 1 addition & 8 deletions pkg/apis/pipeline/v1alpha1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,7 @@ type TaskRunInputs struct {

// TaskResourceBinding points to the PipelineResource that
// will be used for the Task input or output called Name.
type TaskResourceBinding struct {
PipelineResourceBinding
// Paths will probably be removed in #1284, and then PipelineResourceBinding can be used instead.
// The optional Path field corresponds to a path on disk at which the Resource can be found
// (used when providing the resource via mounted volume, overriding the default logic to fetch the Resource).
// +optional
Paths []string `json:"paths,omitempty"`
}
type TaskResourceBinding = v1alpha2.TaskResourceBinding

// TaskRunOutputs holds the output values that this task was invoked with.
type TaskRunOutputs struct {
Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/pipeline/v1alpha1/taskrun_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,29 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) *apis.FieldError {
}
}

// Deprecated
// check for input resources
if err := ts.Inputs.Validate(ctx, "spec.Inputs"); err != nil {
return err
}

// Deprecated
// check for output resources
if err := ts.Outputs.Validate(ctx, "spec.Outputs"); err != nil {
return err
}

// Validate Resources
if err := ts.Resources.Validate(ctx); err != nil {
return err
}

if err := validateWorkspaceBindings(ctx, ts.Workspaces); err != nil {
return err
}
if err := validateParameters(ts.Params); err != nil {
return err
}

if ts.Timeout != nil {
// timeout should be a valid duration of at least 0.
Expand Down
63 changes: 4 additions & 59 deletions pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 3874ceb

Please sign in to comment.