Preserve plugin config env var names with consecutive underscores #2116

triarius · 2023-05-25T13:26:28Z

When an env var name, that was derived from a plugin config, had consecutive underscores, the consecutive underscores were replaced by a single one. (For later reference, this will be called the underscore collapsing transformation). Now, both versions of the variable before and after this transformation are exported.

We also log a warning about this and log both the deprecated variable name and its replacement.
We need to keep the deprecated version around, as these environment variables are how plugins read their config without depending on a tool like jq to parse the config.
So removing them would mean that plugins could be inadvertently misconfigured, and they would have to be rewritten to start using the replacement environment variables instead.

Implementing this was not that straightforward, as the function that creates the environment slice from the plugin config is recursive and did not take a logger or return errors. It is recursive as that is the most straightforward way to turn a tree like object such as

parameter:
  ci:
    _api_key: /path
    db:
      host: localhost
      port: 5432

Into the list of environment variables, similar to:

PARAMETER_CI_API_KEY=/path
PARAMETER_CI_DB_HOST=localhost
PARAMETER_CI_DB_PORT=5432

I could have adapted it to do something when it hits a leaf node and the path to root had consecutive underscores, but I think that would complicate the recursion too much.

Instead, I just removed the underscore collapsing transformation and added a pass over the result that "fans-out" environment variable names with consecutive underscores into two environment variables with and without the underscore collapsing transformation applied.

So in the example above, there will now be an additional environment variable:

PARAMETER_CI__API_KEY=/path
PARAMETER_CI_API_KEY=/path
PARAMETER_CI_DB_HOST=localhost
PARAMETER_CI_DB_PORT=5432

Also, for passing the errors back to bootstrap, I made two custom error types. It's arguably a little overengineered, but I tried to find a way to use error wrapping and/or errors.Join to do the job, but they weren't powerful enough, or I was holding them wrong. There could be some generics libraries we could import to eliminate a lot of the code written for the error types. I would be glad to do this.

Fixes: #2100

Screenshots

Before

After

…ment variables

When an env var name, that was derived from a plugin config, had consecutive underscores, the consecutive underscores were replaced by a single one. Now, both versions of the variable before and after this transformation are exported.

Previously we ignored errors from this method, but now we just log them. Either way, execution continues, exposing us to the possibility of a nil pointer dereference. Since go is not expressive enough to eliminate the possibility of a nil pointer dereference, we have to be extra careful to ensure we don't propagate nil pointer in this manner.

It is ultimately returned as a map, so the sort order is lost

moskyb · 2023-05-25T23:43:06Z

agent/plugin/plugin.go

+	nonIDCharacterRE        = regexp.MustCompile(`[^a-zA-Z0-9]`)
+	consecutiveHyphenRE     = regexp.MustCompile(`-+`)
+	dashOrSpace             = regexp.MustCompile(`-|\s`)
+	whitespaceRE            = regexp.MustCompile(`\s+`)
+	consecutiveUnderscoreRE = regexp.MustCompile(`_+`)


good renames 👍

moskyb · 2023-05-25T23:48:30Z

agent/plugin/plugin.go

+		noConsecutiveUnderScoreKey := consecutiveUnderscoreRE.ReplaceAllString(k, "_")
+		if k != noConsecutiveUnderScoreKey {
+			envSlice = append(envSlice, fmt.Sprintf("%s=%s", noConsecutiveUnderScoreKey, v))
+			err = errors.Join(err, &DeprecatedNameError{old: noConsecutiveUnderScoreKey, new: k})


:O TIL errors.Join made it into the stdlib

In 1.20. But it wasn't powerful enough for me here :(

For testing, I needed a way to make sure that the expected set of errors is exactly the set of the actual errors. But I could only see how to errors.Is on an error created with errors.Join in a way that allows you to conclude that the expected set of errors is a subset of the actual errors.

bootstrap/bootstrap.go

triarius · 2023-05-25T23:17:02Z

agent/plugin/plugin_test.go

+		`[ { "%s": %s } ]`,
+		"github.com/buildkite-plugins/docker-compose-buildkite-plugin",
+		configJSON,
+	)


This is just cosmetic.

triarius · 2023-05-25T23:17:11Z

agent/plugin/plugin_test.go

+		cfgJSON1,
+		"github.com/buildkite-plugins/docker-compose-buildkite-plugin",
+		cfgJSON2,
+	)


This is just cosmetic.

The subsequent sort would sort the original array

Co-authored-by: Ben Moskovitz <ben@mosk.nz>

The deprecation message isn't something that is actionable by people who create builds (builders). It's only relevant if the plugin is depending on the deprecated variable name, and the builder would not normally be aware of this. Furthermore, it's often the case that the plugin will work fine even though the environment variable name is "deprecated", so this does not need to be a warning.

DrJosh9000 · 2023-05-30T07:33:49Z

agent/plugin/error.go

+func (e *DeprecatedNameErrors) Error() string {
+	builder := strings.Builder{}
+	for i, err := range e.Errors() {
+		_, _ = builder.WriteString(err.Error())


Builder.WriteString (and WriteRune) are documented as returning nil error always, so the underscores seem extra unnecessary here.

I don't think it's too distracting to leave it this way. It makes the life of static analysis tools easier.

agent/plugin/plugin.go

DrJosh9000 · 2023-05-30T07:55:56Z

agent/plugin/error.go

+// Errors returns the contained set of errors in sorted order
+func (e *DeprecatedNameErrors) Errors() []DeprecatedNameError {
+	if e == nil {
+		return []DeprecatedNameError{}


Why not return nil?

Yep, and I made it handle when e.errs is nil too.

…Environment in bootstrap

…ains a nil map in Errors()

moskyb

one tiny whitespace thing, but other than that this LGTM 🎉

bootstrap/bootstrap.go

Co-authored-by: Ben Moskovitz <ben@mosk.nz>

Other languages have much better ways to do this

pda

I haven't looked closely at the implementation, but I like what it does 👍🏼
And it looks like there's good test coverage.

DrJosh9000 · 2023-05-31T04:56:20Z

bootstrap/bootstrap.go

-					"You may be able avoid this by removing consecutive underscore, hypen, or whitespace " +
-					"characters in your plugin configuration.",
-			)
+			b.shell.Logger.Printf("%s", strings.Join([]string{


Just to understand, I'm guessing the problem with using a single string literal is how long it would be?

Yep. Wrapped lines are irritating for many reasons

triarius · 2023-05-31T23:06:17Z

The log message has changed somewhat after all the review. Here's what it looks like now in a very similar context to what was in the original issue. I've expanded the relevant sections, they are not expanded by default.

triarius added 8 commits May 26, 2023 08:37

Check and log error returned from converting plugin config to environ…

14d1d35

…ment variables

Remove some redundant type conversions

6d1568d

Add a t.Parallel and fix formatting of plugin_test

4f2e4f4

Create a custom aggregated error type for ConfigurationToEnvironment

aff68a4

Fix tests for returning old and new env vars

fc0acd4

Add a warning about the deprecation of plugin environment variables

f29a37c

triarius force-pushed the pdp-1022-environment-variable-name-transformation-question branch from 22ff9e4 to f29a37c Compare May 25, 2023 22:38

triarius added 5 commits May 26, 2023 10:27

Factor out fanning out of deprecated env var names into a function

30e3b5a

Add some doc comments

582181a

Remove sorting of env var slice

73ccfde

It is ultimately returned as a map, so the sort order is lost

Compare DeprecatedNameErrors without regard to order

586dcb7

Improve formatting of deprecation warning

1051cb3

triarius force-pushed the pdp-1022-environment-variable-name-transformation-question branch from d474bca to 1051cb3 Compare May 26, 2023 05:11

triarius marked this pull request as ready for review May 26, 2023 05:15

triarius requested review from a team, moskyb, DrJosh9000 and pda May 26, 2023 05:16

moskyb approved these changes May 26, 2023

View reviewed changes

triarius commented May 26, 2023

View reviewed changes

triarius added 2 commits May 27, 2023 09:34

Use a more standard name for a regex

8f67c08

Use stable order for tests

784635f

triarius force-pushed the pdp-1022-environment-variable-name-transformation-question branch 2 times, most recently from 2d3527c to 089db2a Compare May 28, 2023 11:03

triarius and others added 4 commits May 28, 2023 21:23

Factor out errors into own file and write some tests

3045f8b

Fix copying in Errors() method is too shallow

892de50

The subsequent sort would sort the original array

Replace array with set

38919c1

Update bootstrap/bootstrap.go

ed9ae19

Co-authored-by: Ben Moskovitz <ben@mosk.nz>

triarius force-pushed the pdp-1022-environment-variable-name-transformation-question branch 4 times, most recently from 60ff367 to ed9ae19 Compare May 28, 2023 15:04

DrJosh9000 reviewed May 30, 2023

View reviewed changes

agent/plugin/plugin.go Outdated Show resolved Hide resolved

DrJosh9000 reviewed May 30, 2023

View reviewed changes

triarius added 3 commits May 30, 2023 22:16

Update message

f6042f5

Add logging of warning if there is another error from ConfigurationTo…

c7c8cd5

…Environment in bootstrap

Distinguish between when DeprecatedNameErrors is nil and when it cont…

18ad336

…ains a nil map in Errors()

triarius force-pushed the pdp-1022-environment-variable-name-transformation-question branch from 4d36759 to 18ad336 Compare May 30, 2023 12:19

moskyb approved these changes May 30, 2023

View reviewed changes

bootstrap/bootstrap.go Outdated Show resolved Hide resolved

Update bootstrap/bootstrap.go

057255b

Co-authored-by: Ben Moskovitz <ben@mosk.nz>

triarius enabled auto-merge May 31, 2023 00:52

triarius disabled auto-merge May 31, 2023 00:53

triarius added 2 commits May 31, 2023 11:00

Use strings.Join for a multiline string

2711988

Other languages have much better ways to do this

Fix spelling of a method

02fccbf

pda reviewed May 31, 2023

View reviewed changes

DrJosh9000 reviewed May 31, 2023

View reviewed changes

triarius merged commit d10a813 into main May 31, 2023
1 check passed

triarius deleted the pdp-1022-environment-variable-name-transformation-question branch May 31, 2023 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve plugin config env var names with consecutive underscores #2116

Preserve plugin config env var names with consecutive underscores #2116

triarius commented May 25, 2023 •

edited

moskyb May 25, 2023

moskyb May 25, 2023

triarius May 26, 2023

triarius May 28, 2023 •

edited

triarius May 25, 2023

triarius May 25, 2023

DrJosh9000 May 30, 2023

triarius May 31, 2023

DrJosh9000 May 30, 2023

triarius May 31, 2023

moskyb left a comment

pda left a comment

DrJosh9000 May 31, 2023

triarius May 31, 2023

triarius commented May 31, 2023

Preserve plugin config env var names with consecutive underscores #2116

Preserve plugin config env var names with consecutive underscores #2116

Conversation

triarius commented May 25, 2023 • edited

Screenshots

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius May 28, 2023 • edited

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

moskyb left a comment

Choose a reason for hiding this comment

pda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius commented May 31, 2023

triarius commented May 25, 2023 •

edited

triarius May 28, 2023 •

edited