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

Add condition "nodes" that triggers a task everytime a Consul node is updated #358

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ test:
test-integration:
@echo "==> Testing ${NAME} (test suite & integration)"
@go test -count=1 -timeout=80s -tags=integration -cover ./... ${TESTARGS}
.PHONY: test-all
.PHONY: test-integration

# test-e2e runs e2e tests
test-e2e: dev
Expand Down
2 changes: 1 addition & 1 deletion client/terraform_cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ module for task "test-workspace" is missing the "catalog_services" variable, add
}
}
]
}`, // Terraform v0.13/v0.14 output
}`, // Terraform v0.13/v0.14 output
},
{
"warning",
Expand Down
6 changes: 6 additions & 0 deletions config/condition.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ func isConditionNil(c ConditionConfig) bool {
result = v == nil
case *CatalogServicesConditionConfig:
result = v == nil
case *NodesConditionConfig:
result = v == nil
default:
return c == nil || reflect.ValueOf(c).IsNil()
}
Expand Down Expand Up @@ -84,6 +86,10 @@ func conditionToTypeFunc() mapstructure.DecodeHookFunc {
var config ServicesConditionConfig
return decodeConditionToType(c, &config)
}
if c, ok := conditions[nodesConditionTypes]; ok {
var config NodesConditionConfig
return decodeConditionToType(c, &config)
}

return nil, fmt.Errorf("unsupported condition type: %v", data)
}
Expand Down
88 changes: 88 additions & 0 deletions config/condition_nodes.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package config

import "fmt"

const nodesConditionTypes = "nodes"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a compile check for NodesConditionConfig to satisfy the interface ConditionConfig below the const? It helps us keep all of the condition types standardized.

var _ ConditionConfig = (*NodesConditionConfig)(nil)


type NodesConditionConfig struct {
Datacenter *string `mapstructure:"datacenter"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the query parameter Filter is available and supported in the node template, it would be a good feature to expose through the configuration to be used.

I like the omission of the node-meta parameter since it's marked as deprecated for the /v1/catalog/nodes endpoint


services []string
Copy link
Contributor

Choose a reason for hiding this comment

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

I support the idea that the nodes condition doesn't allow setting the services in the task config until we know of a clearer use-case. Though I don't see how this list of services here would be set for this validation during config loading.

If that's the case, can you remove this and add the validation within the TaskConfig.Validate()? There are other cases where the condition specs are checked.

}

func (n *NodesConditionConfig) Copy() ConditionConfig {
if n == nil {
return nil
}

var o NodesConditionConfig
o.Datacenter = n.Datacenter
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit to avoid copying the pointer here to avoid unintended changes between copies and merges.

Suggested change
o.Datacenter = n.Datacenter
o.Datacenter = StringCopy(n.Datacenter)

copy(o.services, n.services)

return &o
}

func (n *NodesConditionConfig) Merge(o ConditionConfig) ConditionConfig {
if n == nil {
if isConditionNil(o) {
return nil
}
return o.Copy()
}

if isConditionNil(o) {
return n.Copy()
}

r := n.Copy()
o2, ok := o.(*NodesConditionConfig)
if !ok {
return r
}

r2 := r.(*NodesConditionConfig)

if o2.Datacenter != nil {
r2.Datacenter = StringCopy(o2.Datacenter)
}

r2.services = append(r2.services, o2.services...)

return r2
}

func (n *NodesConditionConfig) Finalize(services []string) {
if n == nil {
return
}

if n.Datacenter == nil {
n.Datacenter = String("")
}

n.services = services
}

func (n *NodesConditionConfig) Validate() error {
if n == nil {
return nil
}

if len(n.services) != 0 {
return fmt.Errorf("task.services cannot be set when using condition %q", nodesConditionTypes)
}

return nil
}

func (n *NodesConditionConfig) GoString() string {
if n == nil {
return "(*NodesConditionConfig)(nil)"
}

return fmt.Sprintf("&NodesConditionConfig{"+
"Datacenter:%s, "+
"}",
StringVal(n.Datacenter),
)
}
165 changes: 165 additions & 0 deletions config/condition_nodes_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
package config

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestNodesConditionConfig_Copy(t *testing.T) {
cases := []struct {
name string
a *NodesConditionConfig
}{
{
"nil",
nil,
},
{
"empty",
&NodesConditionConfig{},
},
{
"fully_configured",
&NodesConditionConfig{
Datacenter: String("dc2"),
},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
r := tc.a.Copy()
if tc.a == nil {
// returned nil interface has nil type, which is unequal to tc.a
assert.Nil(t, r)
} else {
assert.Equal(t, tc.a, r)
}
})
}
}

func TestNodesConditionConfig_Merge(t *testing.T) {
cases := []struct {
name string
a *NodesConditionConfig
b *NodesConditionConfig
r *NodesConditionConfig
}{
{
"nil_a",
nil,
&NodesConditionConfig{},
&NodesConditionConfig{},
},
{
"nil_b",
&NodesConditionConfig{},
nil,
&NodesConditionConfig{},
},
{
"nil_both",
nil,
nil,
nil,
},
{
"empty",
&NodesConditionConfig{},
&NodesConditionConfig{},
&NodesConditionConfig{},
},
{
"datacenter_overrides",
&NodesConditionConfig{Datacenter: String("same")},
&NodesConditionConfig{Datacenter: String("different")},
&NodesConditionConfig{Datacenter: String("different")},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a couple of test cases for completion

Suggested change
},
},
{
"datacenter_empty_one",
& NodesConditionConfig{Datacenter: String("same")},
& NodesConditionConfig{},
& NodesConditionConfig{Datacenter: String("same")},
},
{
"datacenter_empty_two",
& NodesConditionConfig{},
& NodesConditionConfig{Datacenter: String("same")},
& NodesConditionConfig{Datacenter: String("same")},
},

}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
r := tc.a.Merge(tc.b)
if tc.r == nil {
// returned nil interface has nil type, which is unequal to tc.r
assert.Nil(t, r)
} else {
assert.Equal(t, tc.r, r)
}
})
}
}

func TestNodesConditionConfig_Finalize(t *testing.T) {
cases := []struct {
name string
s []string
i *NodesConditionConfig
r *NodesConditionConfig
}{
{
"empty",
[]string{},
&NodesConditionConfig{},
&NodesConditionConfig{
Datacenter: String(""),
services: []string{},
},
},
{
"pass_in_services",
[]string{"api"},
&NodesConditionConfig{},
&NodesConditionConfig{
Datacenter: String(""),
services: []string{"api"},
},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
tc.i.Finalize(tc.s)
assert.Equal(t, tc.r, tc.i)
})
}
}

func TestNodesConditionConfig_Validate(t *testing.T) {
cases := []struct {
name string
expectErr bool
services []string
}{
{
"nil",
false,
nil,
},
{
"empty",
false,
[]string{},
},
{
"services set",
true,
[]string{"api"},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
n := &NodesConditionConfig{}
n.Finalize(tc.services)
err := n.Validate()
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}
52 changes: 52 additions & 0 deletions config/condition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,58 @@ task {
nonexistent_field = true
}
}`,
},
{
"nodes: empty",
false,
&NodesConditionConfig{
Datacenter: String(""),
services: []string{},
},
"config.hcl",
`
task {
name = "condition_task"
source = "..."
services = []
condition "nodes" {}
}
`,
},
{
"nodes: happy path",
false,
&NodesConditionConfig{
Datacenter: String("dc2"),
services: []string{},
},
"config.hcl",
`
task {
name = "condition_task"
source = "..."
services = []
condition "nodes" {
datacenter = "dc2"
}
}
`,
},
{
"nodes: unsupported field",
true,
nil,
"config.hcl",
`
task {
name = "condition_task"
source = "..."
services = []
condition "nodes" {
nonexistent_field = true
}
}
`,
},
{
"error: two conditions",
Expand Down
4 changes: 4 additions & 0 deletions driver/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ func (t *Task) configureRootModuleInput(input *tftmpl.RootModuleInputData) {
condition = &tftmpl.ServicesCondition{
Regexp: *v.Regexp,
}
case *config.NodesConditionConfig:
condition = &tftmpl.NodesCondition{
Datacenter: *v.Datacenter,
}
default:
// expected only for test scenarios
log.Printf("[WARN] (driver.terraform) task '%s' condition config unset."+
Expand Down
Loading