Execute all convert plugins every time.

[captncraig]

Our use-case is that we are using the built-in jsonnet functionality AND the paths-changed extension.

The jsonnet is working well, but the second extension is never getting called. This allows all convert plugins to be executed, even if an earlier one returns a valid config. This lets you chain together multiple conversion plugins.

In the future, I'd like to extend this further to support multiple remote extensions, each with their own secrets. But for now, this would unblock the workflow I want to do.

See https://discourse.drone.io/t/multiple-configuration-extensions/7724

[CLAassistant]

All committers have signed the CLA.

[jvrplmlmn]

@tboerger since this is approved, could it be merged?

[tboerger]

That's something I want to get approved by @bradrydzewski as well

[Upgreydd]

@bradrydzewski I'm testing this for a few days and works well. Thank you @captncraig :+1:

[tonglil]

Facing the same issue described in the PR description. Using the server jsonnet extension DRONE_JSONNET_ENABLED=true and the same paths changed extension, but the paths changed extension doesn't "work".

[rhiaxion]

This change would be so helpful. Any ETA on getting it released?

[bradrydzewski]

I think in order to enable this functionality we need to augment the API to capture the mimetype of the file, and when a plugin converts a file it should be sure to update the mimetype.

When chaining conversion plugins together, a plugin knows (for example) the original file may have been jsonnet by looking at the file suffix. If we enable chaining, a plugin will no longer be able to look at the file suffix to determine the type, since a previous plugin in the chain could have changed the content type.

+	const (
+		ContentTypeYaml     = "text/yaml"
+		ContentTypeJsonnet  = "text/x-jsonnet"
+		ContentTypeStarlark = "text/x-starlark"
+	)

	Config struct {
		Data string `json:"data"`
+		Type string `json:"type"`
	}

We may then want to update some of the built-in conversion plugins, such as the jsonnet and starlark plugins, just to be safe and prevent unforeseen regressions.

	// if the file extension is not jsonnet we can
	// skip this plugin by returning zero values.
	if strings.HasSuffix(req.Repo.Config, ".jsonnet") == false {
		return nil, nil
	}

+	// if the file has already been converted, and is no
+	// longer of type jsonnet, skip this plugin by returning
+	// zero values.
+	if req.Config.Type != "" && req.Config.Type != core.ContentTypeJsonnet {
+		return nil, nil	
+	}

...

	return &core.Config{
		Data: buf.String(),
+		Type: core.ContentTypeYaml,
	}, nil

In the mean time, we have a documented workaround here.

[raphendyr]

@bradrydzewski sorry to ping here, but I didn't know or find any better place. Any idea if that kind of interface change is planned or on a roadmap. As a change, it seems to be really simple, but I get that it requires some time for all the plugins to update.

Having an idea about that would help to make long (or short) term plans for a project I'm working in. We use the same combination of jsonnet and paths plugin.

[georgekaz]

In the mean time, we have a documented workaround here.

Hi @bradrydzewski . As this isn't merged, I was looking at your suggested workaround, but now that the jsonnet plugin is deprecated I wondered how you'd recommend to still achieve this. We're trying to use paths-changed and jsonnet together like the OP.

Is a version of this PR likely to ever get merged? Thanks

[bradrydzewski]

I think the best path forward is to place this behind a feature flag which I have done in https://github.com/harness/drone/commit/62f108668081fe6fcad5da23724c8c71d3d57d65. I think this is a good compromise because it means people can opt-in and start using this feature today without the risk of introducing a regression as described in this comment.

You can start using this feature today by pulling the latest docker image and setting DRONE_CONVERT_MULTI=true. This should also be available in our next minor release which is scheduled for end of next week.

[tonglil]

You can start using this feature today

Can someone confirm if my understanding is correct?

Based on what I see in the links and in the original thread, what is "working today" if you set DRONE_CONVERT_MULTI=true is: built-in jsonnet with https://github.com/meltwater/drone-convert-pathschanged?

[wez]

FWIW, I found today that setting DRONE_CONVERT_MULTI=true and DRONE_STARLARK_ENABLED=true on the drone server allows mixing both starlark and https://github.com/meltwater/drone-convert-pathschanged

[bot2-harness]

This PR has been automatically closed due to inactivity.