Refactor dependsOn into References #262

wryun · 2018-03-21T01:15:37Z

See #233 for some discussion. DO NOT MERGE - need to update doco if we agree. Also, integration-test may be broken (unclear what's happening on first glance).

Why we want this:

avoid problem of messy {{x#x#x}} format of references
adds escapability of references (multiple !s)
if we do want to reintroduce string interpolation, this makes it simpler/cleaner
sets up situation where it's easy to mark particular references as 'secret' (which will cause service catalog processing to make them automatic secrets, etc. etc. - see Create secrets automatically if secrets passed in ServiceInstance/Binding parameter blocks (automated parametersFrom) #233)

Why we don't want this:

verbosity for simple cases
we 'hack' a simple dependsOn (without references), which looks a bit ugly (maybe?). Could instead have a nested structure (e.g. resource then a list of 'names' that we extract from it)
not backwards compatible (will need to change voyager once we merge)
there is a nicer solution we haven't considered

ash2k · 2018-03-21T01:26:28Z

pkg/apis/smith/v1/types.go


 	Spec ResourceSpec `json:"spec"`
 }

+// +k8s:deepcopy-gen=true


Is this needed given that there is a method for it below?

I would say the other way around. Is there a reason for manually defining a DeepCopyInto method, if it should be generated based on this annotation?

Yes, that's what's happening here.

ash2k · 2018-03-21T01:27:35Z

pkg/apis/smith/v1/zz_generated.deepcopy.go

-		for i := range *in {
-			(*in)[i].DeepCopyInto(&(*out)[i])
-		}
+		copy(*out, *in)


ash2k · 2018-03-21T01:32:10Z

pkg/controller/resource_sync_task.go

-		if !st.processedResources[dependency].isReady() {
-			notReadyDependencies = append(notReadyDependencies, dependency)
+	// No len here because dependencies can occur more than once in reference list
+	notReadyDependenciesSet := make(map[smith_v1.ResourceName]bool)


The convention is to use struct{} as value for sets. This makes it more explicit that you don't care about the value.

ash2k · 2018-03-21T01:36:44Z

pkg/controller/spec_processor.go

 )

 var (
-	reference = regexp.MustCompile(`^\{\{.+}}$`)
+	reference = regexp.MustCompile(`(?s)^(!+)\{(.+)}$`)


For the reader:
https://github.com/google/re2/wiki/Syntax
(?s) - let . match \n (default false)

ash2k · 2018-03-21T01:39:15Z

pkg/controller/spec_processor.go

+func NewExamplesSpec(references []smith_v1.Reference) (*SpecProcessor, error) {
+	variables, err := resolveAllReferences(references, func(reference smith_v1.Reference) (interface{}, error) {
+		if reference.Example == nil {
+			return nil, errors.WithStack(&noExampleError{reference.Name})


Prefer specifying field names explicitly even if there is only one field. If you later refactor - add a field of the same type - this will not break the compilation but become a bug silently.

ash2k · 2018-03-21T01:40:32Z

pkg/controller/spec_processor.go

+	for _, reference := range references {
+		// Don't 'resolve' nameless references - they're just being
+		// used to cause dependencies.
+		if reference.Name != "" {


I'd unindent the happy path and rewrite this as if reference.Name == "" { continue }

ash2k · 2018-03-21T01:41:38Z

pkg/controller/spec_processor.go

 	smith_v1 "github.com/atlassian/smith/pkg/apis/smith/v1"
 	"github.com/atlassian/smith/pkg/resources"
 	"github.com/pkg/errors"
+	"go.uber.org/multierr"


I think we use some multierr package from client-go somewhere already. Lets pick one =)

I can't find it...

k8s.io/apimachinery/pkg/util/errors/NewAggregate()

ash2k · 2018-03-21T01:55:26Z

pkg/controller/spec_processor.go

-	if err != nil {
-		return nil, errors.Wrapf(err, "invalid reference at \"%s\"", strings.Join(path, "."))
+	// This means we have multiple '!' at the start, so we're escaping.
+	if len(match[1]) > 1 {


We should ignore pairs of ! and then process the rest if there is an unmatched !, right? This can even be done with regex - match 0+ pairs and then expect an !. Hm, but then those pairs need to be transformed into single !.

Also please add tests for escaping. And we should export a function to escape a string and another one to escape a whole object. Also maybe add a flag to each resource to tell smith whether to look for references in this resource or not. Because the choice for programmatic construction is between 1) escape each one even if it does not contain references by construction 2) set a flag marking the resource as not containing references. WDYT?

Agree re:tests. I would like to see examples of such escaping format.

Sorry, I thought I added tests.
I don't see a problem with this method of escaping though - it should just work, aside from the programmatic construction case... my preference here, rather than a flag, would be to look for references IFF there are references (i.e. if sp.variables is non-empty).

(yes, it's a little magical, but it keeps the base case clean)

On further thought, I think the current behaviour is the correct one. Programmatic users should just have to deal with escaping (it's relatively straightforward, as are the error cases... we also have no cases currently).

I will add the functions to escape strings/objects in a separate PR?

nilebox

Overall I like the direction of this PR.
See the comments.

nilebox · 2018-03-26T04:25:26Z

pkg/apis/smith/v1/types.go


 	Spec ResourceSpec `json:"spec"`
 }

+// +k8s:deepcopy-gen=true


I would say the other way around. Is there a reason for manually defining a DeepCopyInto method, if it should be generated based on this annotation?

nilebox · 2018-03-26T04:41:35Z

pkg/controller/resource_sync_task.go

-	for _, dependency := range res.DependsOn {
-		if !st.processedResources[dependency].isReady() {
-			notReadyDependencies = append(notReadyDependencies, dependency)
+	// No len here because dependencies can occur more than once in reference list


I don't think this is a justification for not passing an initial capacity at all.
you can easily set more key-value pairs than initially allocated, i.e. using len(res.References) is actually fine.

? But then I over-allocate, potentially dramatically?
(it's not under-allocation that will happen, it's over-allocation)

ah I see. I doubt there is a huge memory footprint for map[string]struct{}.
And also I don't see any reason for a dependency occuring more than once in reference list.

(it will occur more than once because of the path problem)

nilebox · 2018-03-26T04:46:42Z

pkg/controller/resource_sync_task.go

 		}
 	}
+	notReadyDependencies := make([]smith_v1.ResourceName, 0, len(notReadyDependenciesSet))


you can allocate a fixed size array instead here:

notReadyDependencies := make([]smith_v1.ResourceName, len(notReadyDependenciesSet)) i := 0 for resourceName, _ := range notReadyDependenciesSet { notReadyDependencies[i] = resourceName i++ }

Isn't this equivalent, just more verbose and error prone?

(maybe append is slightly slower? You'd think it would be optimised away. But I'm using the same array...)

I think append is inherently safer and reads nicer =)

nilebox · 2018-03-26T04:47:09Z

pkg/controller/resource_sync_task.go

 		}
 	}
+	notReadyDependencies := make([]smith_v1.ResourceName, 0, len(notReadyDependenciesSet))
+	for resourceName, _ := range notReadyDependenciesSet {


_ is redundant here. Golang magic.

Heh, I find it ugly to leave out. Ok, will change.

nilebox · 2018-03-26T04:59:30Z

it/workflow_test.go

@@ -69,8 +69,10 @@ func TestWorkflow(t *testing.T) {
 		Spec: smith_v1.BundleSpec{


(for myself and other reviewers)
equivalent in YAML:

spec: resources: - name: config1res references: - resource: secret2res spec: object: ...

@wryun if I understand correctly, such minimal reference is only suitable for creation ordering, and not for injecting values from the other objects right?

Yes. You need to specify name/path if you want an actual value.

nilebox · 2018-03-26T05:14:05Z

pkg/controller/spec_processor.go

+	ar := make(map[smith_v1.ReferenceName]interface{}, len(references))
+	var finalerr error
+	for _, reference := range references {
+		// Don't 'resolve' nameless references - they're just being


Not sure if I like the idea of this behavior depending on whether name is defined or not. See comments above about name vs resource.

I'd rather remove name or make it optional.
And we can resolve all references without checking for name != "", this optimization seems redundant to me.

I'm not sure what you mean here. Maybe talk in person.

TL;DR just remove this block:

// used to cause dependencies. if reference.Name == "" { continue }

and I propose to delete the name field completely.

nilebox · 2018-03-26T05:17:18Z

pkg/controller/spec_processor.go

-	if err != nil {
-		return nil, errors.Wrapf(err, "invalid reference at \"%s\"", strings.Join(path, "."))
+	// This means we have multiple '!' at the start, so we're escaping.
+	if len(match[1]) > 1 {


Agree re:tests. I would like to see examples of such escaping format.

nilebox · 2018-03-26T05:18:36Z

pkg/controller/spec_processor.go

 	}
 	if fieldValue == nil {
-		return nil, errors.Errorf("field not found: %s", selector)
+		return nil, errors.Errorf("field not found: %s", reference.Path)


nilebox · 2018-03-26T05:20:13Z

pkg/controller/spec_processor_test.go

+		{
+			Name:     "res1aslice",
+			Resource: "res1",
+			Path:     "a.slice[?(@.label==\"label2\")].value",


So we are not even referring to name inside the path or anywhere else, right? What's the purpose of name then?

nilebox · 2018-03-26T05:21:26Z

pkg/controller/spec_processor_test.go

 	obj := map[string]interface{}{
 		"ref": map[string]interface{}{
-			"slice":  "{{res1#a.slice[?(@.label==\"label2\")].value}}",
-			"string": "{{res1#a.string}}",
+			"slice":  "!{res1aslice}",


ok I see, we use name there.

ash2k · 2018-03-26T06:39:19Z

@nilebox take a look at the proposal here #233 (comment) - this should clarify what is going on in this PR.

ash2k · 2018-03-26T06:40:58Z

@wryun would be great to also update all the documentation to use the new syntax introduced in this PR.

ash2k · 2018-03-26T06:43:52Z

@wryun To keep this PR smaller I suggest to remove escaping functionality from it. And then add it in a later PR. We need to discuss it first, I have a feeling it might require more code than we have here.

See #233 for discussion

ash2k

Some minor comments from me which I mostly addressed myself 😛

ash2k · 2018-03-29T09:04:38Z

pkg/apis/smith/v1/types.go

+// Refer to a part of another object
+type Reference struct {
+	Name     ReferenceName `json:"name,omitempty"`
+	Resource ResourceName  `json:"resource"`


This should probably be ResourceName ResourceName. But then it is a bit too verbose. WDYT?

ash2k · 2018-03-29T09:15:09Z

pkg/controller/spec_processor.go

+
+		resolvedRef, err := resolveReference(reference)
+		if err != nil {
+			finalerr = multierr.Append(finalerr, err)


Is this by design the there is no continue here to avoid adding invalid resolved ref into the map?

ash2k · 2018-03-29T09:55:28Z

The remaining work here is to fix the docs.

ash2k · 2018-04-05T09:44:16Z

I'm going to merge this because I reeeally want to start tinkering with the "plugable controller" idea 😛
I've created #267 to track needed documentation work.

wryun · 2018-04-05T09:46:00Z

Ah, sorry. I had intended to get to it today, but house painting makes me lazy (actually, most things make me lazy).

wryun requested review from ash2k, wathrog, scottgreenup, nilebox, amckague and ychen-atlassian March 21, 2018 01:15

wryun mentioned this pull request Mar 21, 2018

Some nicer way to base64decode and jsondecode #263

Open

ash2k suggested changes Mar 21, 2018

View reviewed changes

nilebox reviewed Mar 26, 2018

View reviewed changes

ash2k assigned wryun Mar 26, 2018

ash2k added the enhancement label Mar 26, 2018

James Haggerty and others added 4 commits March 29, 2018 20:02

Refactor dependsOn into References

7ced147

See #233 for discussion

Minor PR fixups to References

4fceff8

Removing escaping (subsequent PR)

a864e43

Misc fixes from PR review

1bc5334

ash2k force-pushed the refactor-dependson branch from c749934 to 1bc5334 Compare March 29, 2018 09:45

ash2k approved these changes Mar 29, 2018

View reviewed changes

ash2k mentioned this pull request Apr 5, 2018

Update documentation #267

Closed

ash2k merged commit 4cb5cd0 into master Apr 5, 2018

ash2k deleted the refactor-dependson branch April 5, 2018 09:44

		@@ -69,8 +69,10 @@ func TestWorkflow(t *testing.T) {
		Spec: smith_v1.BundleSpec{

Refactor dependsOn into References #262

Refactor dependsOn into References #262

Conversation

wryun commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wryun Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wryun Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Mar 26, 2018

ash2k commented Mar 26, 2018

ash2k commented Mar 26, 2018

ash2k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Mar 29, 2018

ash2k commented Apr 5, 2018

wryun commented Apr 5, 2018 • edited Loading

wryun commented Mar 21, 2018 •

edited

Loading

wryun Mar 26, 2018 •

edited

Loading

nilebox left a comment •

edited

Loading

wryun Mar 26, 2018 •

edited

Loading

wryun commented Apr 5, 2018 •

edited

Loading