-
Notifications
You must be signed in to change notification settings - Fork 699
Matrix custom env vars #3376
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
Matrix custom env vars #3376
Conversation
206c9f8
to
7105a5f
Compare
This reverts commit 4d0ffd6.
7105a5f
to
7a9a174
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks like the logic is OK, but can we simplify the solution.
What's the motivation for these custom formats? It seems to introduce quite a bit of complexity if it's not strictly necessary.
.github/workflows/benchmarks.yml
Outdated
linux_env_var_list_separator: | ||
type: string | ||
description: "Separator between environment variable pairs for Linux." | ||
default: "," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the PR description:
To attempt to avoid issues with this string concatenation the change also allows the user to specify custom list entry and key-value-pair separators.
Do we really think this is worth the added complexity. Why would it not be feasible for this layer (the GitHub YAML) to mandate that env vars are passed as a comma-separated key=value
pairs?
Or better yet,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree with this, since this ends up as JSON anyway I've pushed a change which instead takes a JSON map to define the key-value pairs.
.github/workflows/macos_tests.yml
Outdated
- id: generate-matrix | ||
run: | | ||
# Parse environment variables using the parse_env_vars script | ||
env_vars_json=$(curl -s --retry 3 https://raw.githubusercontent.com/apple/swift-nio/main/scripts/parse_env_vars.sh | ENV_VARS="$MATRIX_ENV_VARS" ENV_VAR_PAIR_SEPARATOR="$MATRIX_ENV_VAR_PAIR_SEPARATOR" ENV_VAR_LIST_SEPARATOR="$MATRIX_ENV_VAR_LIST_SEPARATOR" bash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this warrant a script? Potentially if we standardise of key1=value1,key2=value2,...
this could be a simple jq
command.
Something like this?
echo "a=b,c=d" | jq -R 'split(",") | map(split("=")) | map({key: .[0], value: .[1]}) | from_entries'
persist-credentials: false | ||
submodules: true | ||
- name: Export environment variables | ||
if: ${{ matrix.config.env != '' && matrix.config.env != '{}'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, isn't the schema defining env
as type object
? If so, is the first boolean statement in this expression necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatedly, I think we're really starting to grow beyond the current method. I understand that "inputs" only have restricted types (string, choice, boolean). Clearly it'd be out of scope for this PR, but would moving these to be Actions-proper support having richer inputs? e.g.
uses: our-thing/thing-action@v1
with:
env:
- this: is
- a: proper
- map: type
- just: like
- normal: env
This reverts commit 2c3c5ff.
Allow the specification of custom environment variables in our matrix workflow and those which use it.
Motivation:
It can be useful to supply custom environment variables to control build and runtime options.
Modifications:
Allow users to supply a string of custom environment variable key-value pairs which are supplied to the docker container running the matrix commands.
To attempt to avoid issues with this string concatenation the change also allows the user to specify custom list entry and key-value-pair separators.
To avoid parameter bloat the env vars are set for all Swift versions, hopefully that's sufficient for now.
Result:
Adopters can
Examples of this working on Linux and Windows, note the presence of the test environment variables I injected:
e.g.