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

Execute all convert plugins every time. #2994

Closed
wants to merge 1 commit into from

Conversation

captncraig
Copy link

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
Copy link

CLAassistant commented Jul 17, 2020

CLA assistant check
All committers have signed the CLA.

@jvrplmlmn
Copy link

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

@tboerger
Copy link

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

@Upgreydd
Copy link

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

@tonglil
Copy link

tonglil commented Jan 20, 2021

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
Copy link

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

@bradrydzewski
Copy link

bradrydzewski commented Mar 10, 2021

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
Copy link

@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
Copy link

georgekaz commented Mar 14, 2022

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
Copy link

I think the best path forward is to place this behind a feature flag which I have done in 62f1086. 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
Copy link

tonglil commented Jun 14, 2022

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
Copy link

wez commented Sep 18, 2023

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

@hitesharinga hitesharinga changed the base branch from master to drone October 4, 2023 02:44
@bot2-harness
Copy link
Collaborator

This PR has been automatically closed due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet