Add library package for resolving devfile parents and plugins #74

amisevsk · 2021-03-19T23:36:21Z

What does this PR do?

This PR is a port of the pkg/library/flatten library in the DevWorkspace operator. I initially wrote it to quickly add support for plugins in the DevWorkspace Operator, but it should be useful to anyone working with devfiles.

This PR starts with a relatively straightforward port of the current work in DWO. For additional context in developing this, see PRs devfile/devworkspace-operator#240 and devfile/devworkspace-operator#269.

The basic API defined here is

func ResolveDevWorkspace(workspace *devfile.DevWorkspaceTemplateSpec, tooling ResolverTools) (*devfile.DevWorkspaceTemplateSpec, error)

which takes a DevWorkspaceTemplateSpec and returns the equivalent DevWorkspaceTemplateSpec with all of the plugins and parents resolved and merged/overridden. The ResolverTools struct contains all the interfaces needed to actually grab plugins from outside the current binary:

type ResolverTools struct {
	// DefaultNamespace is the default namespace to use for resolving Kubernetes ImportReferences that do not include one
	DefaultNamespace string
	// DefaultRegistryURL is the default registry URL to use when a component specifies an id but not registryURL
	DefaultRegistryURL string
	// Context is the context used for making Kubernetes or HTTP requests
	Context context.Context
	// K8sClient is the Kubernetes client instance used for interacting with a cluster
	K8sClient client.Client
	// HttpClient is the HTTP client used for making network requests when resolving plugins or parents.
	HttpClient network.HTTPGetter
}

If any field is nil or empty, an error is returned only if it is used. This means using this library to support only Kubernetes-type references is as simple as providing no HttpClient and checking the error returned (this could probably be improved with error types).

One issue we ran into in DWO is that it can be hard to determine where e.g. a component in the flattened devfile came from. To help with this, the merge process adds attributes library.devfile.io/imported-by=source to Components, Commands, Projects, and StarterProjects that are not in the original devfile. For plugins <source> is the name of the plugin component that imported them, for parents, source is parent.

The code is fairly well-tested (coverage >90%), and there's not actually that many non-test lines of code here (~550 lines of actual code, the rest is tests).

I've tried to keep the commit history in this PR readable commit-by-commit (with helpful commit messages), and tried to keep test cases separate from functional changes.

What issues does this PR fix or reference?

I don't know if there is an issue for this

Is your PR tested? Consider putting some instruction how to test your changes

We use this code regularly in the DevWorkspace Operator without issue. Running tests on the changes directly can be done by executing

go test ./pkg/flatten/...

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Port code from DevWorkspace Operator as of commit 95cb199a135518aa9909c8868b1342b85c509523. This version of the flattening library is designed to work with devworkspaces and the operator and needs to be adapted. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Ports tests from DevWorkspace Operator repo as of commit 95cb199a135518aa9909c8868b1342b85c509523 Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Make the utility functions used for resolving plugins by ID, URI, or Kubernetes reference not depend on the PluginComponent type and instead take the relevant information directly. This is required to allow reusing these functions for resolving Devfile parents Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add support for resolving devfile parents in same way we resolve plugins (supporting id, URI, and kubernetes reference) Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

If attempting to unmarshal a network response to a devfile results in a struct without a schemaVersion, attempt to unmarshal to a DevWorkspace or DevWorkspaceTemplate instead. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Rename plugin-related struct fields in tests to be generic, to allow them to hold any resources referenced. Required for reusing these structs cleanly in parent tests. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Make flatten library work without an HTTP or Kubernetes client to allow supporting only kubernetes or http references. If attempting to flatten a devfile that contains an import that doesn't have the required client, return a descriptive error. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Adds test cases to cover: - Using resolver when HttpClient or K8sClient are nil - Resolving Kubernetes resources when no namespace can be determined - Resolving resources by ID when no registryURL can be determined Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add attribute 'library.devfile.io/imported-by: parent' to Components, Commands, Projects, and StarterProjects imported via a parent in the original devfile, to help track where resources in the resolved Devfile came from Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

openshift-ci-robot · 2021-03-19T23:36:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [amisevsk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

amisevsk · 2021-03-22T18:45:01Z

Some additional thoughts on this PR:

The approach here may be complicated by Remove plugins from the Devfile schema api#333 since we'll likely not have a common struct (DevWorkspaceTemplateSpec) to represent both the spec of a devfile and DevWorkspace
The devworkspace operator currently uses the EPL 2.0 license, we need to figure out how to reconcile this with Apache (I believe we want to switch DWO to Apache as well). Files existing in DWO currently have an EPL license header that will probably need to be removed.

yangcao77 · 2021-03-22T21:01:15Z

Since plugins is going to be removed from devfile by devfile/api#333, we decided not to add any more plugins support into devfile/library.

In addition, the parser in library now contains it's own logic to get flattened devfile, and support parent reference using uri & id. It supports more than what DWO currently has, i.e. relative uri(local path & URL), and multiple registry URLs can be passed in to parser.

Kubernetes CRD is the one we want to add support to. We might take some pieces from this PR, which resolves Kubernetes CRD reference, and combine with the existing logic

amisevsk · 2021-03-22T23:25:55Z

Since plugins is going to be removed from devfile by devfile/api#333, we decided not to add any more plugins support into devfile/library

Well, plugins are being removed from the devfile, but they are staying in the DevWorkspace spec for now.

In addition, the parser in library now contains it's own logic to get flattened devfile, and support parent reference using uri & id. It supports more than what DWO currently has, i.e. relative uri(local path & URL), and multiple registry URLs can be passed in to parser.

I think our set of needs is different :) -- there's no relative URIs in a controller.

sleshchenko

The code LGTM.

A bit later will do another round of review to take a look test cases as well.

sleshchenko · 2021-03-23T13:19:03Z

pkg/flatten/flatten.go

+// ResolveDevWorkspace takes a DevWorkspaceTemplateSpec and returns a "resolved" version of it -- i.e. one where all plugins and parents
+// are inlined as components.
+// TODO:
+// - Implement flattening for DevWorkspace parents


I see resolveParentComponent func, isn't it already done?

Forgot to remove the TODO after implementing :)

sleshchenko · 2021-03-23T13:21:28Z

pkg/flatten/testdata/annotate/annotate-devfile-with-importing-source.yaml

+      - id: plugin-command
+        attributes:
+          library.devfile.io/imported-by: test-plugin
+


Suggested change

sleshchenko · 2021-03-23T13:23:23Z

pkg/flatten/testdata/general/fail-nicely-when-no-http-client-provided_id.yaml

+          id: test/plugin
+
+output:
+  errRegexp: "cannot resolve resources by id: no HTTP client provided"


Suggested change

errRegexp: "cannot resolve resources by id: no HTTP client provided"

errRegexp: "cannot resolve resources by id: no HTTP client provided"

the value of my review increases after I commented all end-file empty lines )
Here and a few places below they are missing

sleshchenko · 2021-03-23T13:28:34Z

pkg/flatten/flatten.go

+// are inlined as components.
+// TODO:
+// - Implement flattening for DevWorkspace parents
+func ResolveDevWorkspace(workspace *devfile.DevWorkspaceTemplateSpec, tooling ResolverTools) (*devfile.DevWorkspaceTemplateSpec, error) {


I assume it may simplify testing for clients if such functionality would be provided with interface. Like DevWorkspaceResolver.
Then here we would not need to pass resolverTooling which is good thing as well I think, instead we'll have something like:

r := resolver.NewWithTooling(tooling) r.resolveDevWorkspace(tooling)

But it's not a strong suggestion, just assumption.

sleshchenko · 2021-03-23T13:38:51Z

A bit later will do another round of review to take a look test cases as well.

I'm taking my words back ) I don't plan to do more detailed review since it does not seem this PR will be merged due plugins. If it's changed, please request my review one more time.

maysunfaisal · 2021-03-23T14:31:21Z

go.mod

 	github.com/google/go-cmp v0.4.0
 	github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7
 	github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348
 	github.com/mattn/go-colorable v0.1.2 // indirect
 	github.com/mattn/go-isatty v0.0.12 // indirect
-	github.com/openshift/api v0.0.0-20200930075302-db52bc4ef99f
+	github.com/openshift/api v3.9.0+incompatible


I think we should stick with openshift/api version 4.6 or higher. I had to explicitly update to a newer version because openshift/console and openshift/odo uses newer versions and there is a breaking dependency change when going from version 3.9 to 4.0 and above

Yeah, I'm not sure why this got pinned to 3.9.0 -- I assume it's Go's automatic "upgrading" (3.9 is bigger than 0.0?)

I'll fix this.

yangcao77 · 2021-03-23T16:02:31Z

Well, plugins are being removed from the devfile, but they are staying in the DevWorkspace spec for now.

Fair enough. So library is not going to support plugin from devfile spec perspective, but if the devWorkspaceTemplate specified under parent's kubernetes CRD, we will parse and put it in the flattened devfile. If DWO needs more plugin support, it will need to be handled in DWO specifically. Does that sound good to you?

I think our set of needs is different :) -- there's no relative URIs in a controller.

The existing library also accepts absolute URL address as DWO expected, the relative URIs support is just an extra.

maysunfaisal · 2021-03-24T21:12:08Z

pkg/flatten/flatten.go

+		// Search in default namespace if namespace ref is unset
+		if parent.Kubernetes.Namespace == "" {
+			parent.Kubernetes.Namespace = tooling.DefaultNamespace
+		}


probably isnt reqd since this check and assignment are done in resolveElementByKubernetesImport(), same for plugins

amisevsk · 2021-03-29T19:35:04Z

Closing this PR as this is not the approach devfile/library will take.

amisevsk added 13 commits March 19, 2021 16:48

Update go.mod to support controller-runtime 0.6.3

6dcba87

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Port DevWorkspace Operator tests for flattening library

dedbf01

Ports tests from DevWorkspace Operator repo as of commit 95cb199a135518aa9909c8868b1342b85c509523 Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add support for resolving devfile parents in same way as plugins

c76d821

Add support for resolving devfile parents in same way we resolve plugins (supporting id, URI, and kubernetes reference) Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Clean up devfile object handling to use built-in struct

0dd4830

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add test cases to cover devworkspace(templates) on URIs

8d43036

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Modify test struct names to accomodate parent tests

ac61ee0

Rename plugin-related struct fields in tests to be generic, to allow them to hold any resources referenced. Required for reusing these structs cleanly in parent tests. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add tests to cover attributes on merged devfiles

ff01c26

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk requested review from sleshchenko, JPinkney and yangcao77 March 19, 2021 23:36

openshift-ci-robot requested a review from elsony March 19, 2021 23:36

openshift-ci-robot added the approved label Mar 19, 2021

sleshchenko reviewed Mar 23, 2021

View reviewed changes

maysunfaisal reviewed Mar 23, 2021

View reviewed changes

amisevsk mentioned this pull request Mar 24, 2021

Add support for DevWorkspaces with parents devfile/devworkspace-operator#346

Merged

maysunfaisal reviewed Mar 24, 2021

View reviewed changes

amisevsk closed this Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add library package for resolving devfile parents and plugins #74

Add library package for resolving devfile parents and plugins #74

amisevsk commented Mar 19, 2021

openshift-ci-robot commented Mar 19, 2021

amisevsk commented Mar 22, 2021

yangcao77 commented Mar 22, 2021

amisevsk commented Mar 22, 2021

sleshchenko left a comment

sleshchenko Mar 23, 2021

amisevsk Mar 23, 2021

sleshchenko Mar 23, 2021

sleshchenko Mar 23, 2021 •

edited

Loading

sleshchenko Mar 23, 2021

sleshchenko commented Mar 23, 2021

maysunfaisal Mar 23, 2021

amisevsk Mar 25, 2021

yangcao77 commented Mar 23, 2021 •

edited

Loading

maysunfaisal Mar 24, 2021

amisevsk commented Mar 29, 2021

	errRegexp: "cannot resolve resources by id: no HTTP client provided"
	errRegexp: "cannot resolve resources by id: no HTTP client provided"

Add library package for resolving devfile parents and plugins #74

Add library package for resolving devfile parents and plugins #74

Conversation

amisevsk commented Mar 19, 2021

What does this PR do?

What issues does this PR fix or reference?

Is your PR tested? Consider putting some instruction how to test your changes

openshift-ci-robot commented Mar 19, 2021

amisevsk commented Mar 22, 2021

yangcao77 commented Mar 22, 2021

amisevsk commented Mar 22, 2021

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko Mar 23, 2021

Choose a reason for hiding this comment

amisevsk Mar 23, 2021

Choose a reason for hiding this comment

sleshchenko Mar 23, 2021

Choose a reason for hiding this comment

sleshchenko Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

sleshchenko Mar 23, 2021

Choose a reason for hiding this comment

sleshchenko commented Mar 23, 2021

maysunfaisal Mar 23, 2021

Choose a reason for hiding this comment

amisevsk Mar 25, 2021

Choose a reason for hiding this comment

yangcao77 commented Mar 23, 2021 • edited Loading

maysunfaisal Mar 24, 2021

Choose a reason for hiding this comment

amisevsk commented Mar 29, 2021

sleshchenko Mar 23, 2021 •

edited

Loading

yangcao77 commented Mar 23, 2021 •

edited

Loading