Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Configurable operation timeouts #191

Merged
merged 2 commits into from
Jan 6, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
19 changes: 19 additions & 0 deletions pkg/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package config

import (
"context"
"time"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/pkg/errors"
Expand Down Expand Up @@ -184,6 +185,21 @@ func (s *Sensitive) AddFieldPath(tf, xp string) {
s.fieldPaths[tf] = xp
}

// OperationTimeouts allows configuring resource operation timeouts:
// https://www.terraform.io/language/resources/syntax#operation-timeouts
// Please note that, not all resources support configuring timeouts.
type OperationTimeouts struct {
Read *time.Duration
Create *time.Duration
Update *time.Duration
Delete *time.Duration
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Read *time.Duration
Create *time.Duration
Update *time.Duration
Delete *time.Duration
Read time.Duration
Create time.Duration
Update time.Duration
Delete time.Duration

I am guessing that you had to use pointers to check whether it's given or not but there is a function (t Time) IsZero() bool we can use to do that. If that's the only reason, we could get rid of TimeDuration(d time.Duration) *time.Duration and work with value types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was the reason and I was thinking that setting timeout to 0 could be a valid scenario, e.g. no timeout. But digging the terraform sdk code, I noticed that there is no such an implementation and probably no timeout does not make sense at all.

So, I would revert pointers but to check zero value I can not use IsZero() because it is on time.Time not time.Duration. Am I missing something?

Otherwise, I will go with the following:

	if t := fp.Config.OperationTimeouts.Read.String(); t != "0s" {
		timeouts["read"] = t
	}

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, I missed that IsZero wouldn't work. I think you can check like fp.Config.OperationTimeouts.Read != 0 since type Duration int64

}

// TimeDuration returns a pointer to a given time.Duration.
func TimeDuration(d time.Duration) *time.Duration {
return &d
}

// Resource is the set of information that you can override at different steps
// of the code generation pipeline.
type Resource struct {
Expand Down Expand Up @@ -212,6 +228,9 @@ type Resource struct {
// databases.
UseAsync bool

// OperationTimeouts allows configuring resource operation timeouts.
OperationTimeouts OperationTimeouts

// ExternalName allows you to specify a custom ExternalName.
ExternalName ExternalName

Expand Down
24 changes: 24 additions & 0 deletions pkg/terraform/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,30 @@ func (fp *FileProducer) WriteMainTF() error {
fp.parameters["lifecycle"] = map[string]bool{
"prevent_destroy": !meta.WasDeleted(fp.Resource),
}

// Add operation timeouts if any timeout configured for the resource
timeouts := map[string]string{}
timeoutsConfigured := false
if fp.Config.OperationTimeouts.Read != nil {
timeouts["read"] = fp.Config.OperationTimeouts.Read.String()
timeoutsConfigured = true
}
if fp.Config.OperationTimeouts.Create != nil {
timeouts["create"] = fp.Config.OperationTimeouts.Create.String()
timeoutsConfigured = true
}
if fp.Config.OperationTimeouts.Update != nil {
timeouts["update"] = fp.Config.OperationTimeouts.Update.String()
timeoutsConfigured = true
}
if fp.Config.OperationTimeouts.Delete != nil {
timeouts["delete"] = fp.Config.OperationTimeouts.Delete.String()
timeoutsConfigured = true
}
if timeoutsConfigured {
turkenh marked this conversation as resolved.
Show resolved Hide resolved
fp.parameters["timeouts"] = timeouts
}

// Note(turkenh): To use third party providers, we need to configure
// provider name in required_providers.
providerSource := strings.Split(fp.Setup.Requirement.Source, "/")
Expand Down
47 changes: 44 additions & 3 deletions pkg/terraform/files_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"path/filepath"
"testing"
"time"

"github.com/crossplane/crossplane-runtime/pkg/meta"
xpfake "github.com/crossplane/crossplane-runtime/pkg/resource/fake"
Expand Down Expand Up @@ -96,8 +97,9 @@ func TestWriteTFState(t *testing.T) {

func TestWriteMainTF(t *testing.T) {
type args struct {
tr resource.Terraformed
s Setup
tr resource.Terraformed
cfg *config.Resource
s Setup
}
type want struct {
maintf string
Expand All @@ -108,6 +110,44 @@ func TestWriteMainTF(t *testing.T) {
args
want
}{
"TimeoutsConfigured": {
reason: "Configured resources should be able to write everything it has into maintf file",
args: args{
tr: &fake.Terraformed{
Managed: xpfake.Managed{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
resource.AnnotationKeyPrivateRawAttribute: "privateraw",
meta.AnnotationKeyExternalName: "some-id",
},
},
},
Parameterizable: fake.Parameterizable{Parameters: map[string]interface{}{
"param": "paramval",
}},
Observable: fake.Observable{Observation: map[string]interface{}{
"obs": "obsval",
}},
},
cfg: config.DefaultResource("terrajet_resource", nil, func(r *config.Resource) {
r.OperationTimeouts = config.OperationTimeouts{
Read: config.TimeDuration(30 * time.Second),
Update: config.TimeDuration(2 * time.Minute),
}
}),
s: Setup{
Requirement: ProviderRequirement{
Source: "hashicorp/provider-test",
Version: "1.2.3",
},
Configuration: nil,
Env: nil,
},
},
want: want{
maintf: `{"provider":{"provider-test":null},"resource":{"":{"":{"lifecycle":{"prevent_destroy":true},"name":"some-id","param":"paramval","timeouts":{"read":"30s","update":"2m0s"}}}},"terraform":{"required_providers":{"provider-test":{"source":"hashicorp/provider-test","version":"1.2.3"}}}}`,
},
},
"Success": {
reason: "Standard resources should be able to write everything it has into maintf file",
args: args{
Expand All @@ -127,6 +167,7 @@ func TestWriteMainTF(t *testing.T) {
"obs": "obsval",
}},
},
cfg: config.DefaultResource("terrajet_resource", nil),
s: Setup{
Requirement: ProviderRequirement{
Source: "hashicorp/provider-test",
Expand All @@ -144,7 +185,7 @@ func TestWriteMainTF(t *testing.T) {
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
fs := afero.NewMemMapFs()
fp, err := NewFileProducer(context.TODO(), nil, dir, tc.args.tr, tc.args.s, config.DefaultResource("terrajet_resource", nil), WithFileSystem(fs))
fp, err := NewFileProducer(context.TODO(), nil, dir, tc.args.tr, tc.args.s, tc.args.cfg, WithFileSystem(fs))
if err != nil {
t.Errorf("cannot initialize a file producer: %s", err.Error())
}
Expand Down