Add GitHub Actions export only shell #910
Conversation
internal/cmd/shell.go
Outdated
| switch target { | ||
| case "bash": | ||
| return Bash | ||
| case "zsh": | ||
| return Zsh | ||
| case "elvish": | ||
| return Elvish | ||
| case "fish": | ||
| return Fish | ||
| case "gzenv": | ||
| return GzEnv | ||
| case "vim": | ||
| return Vim | ||
| case "tcsh": | ||
| return Tcsh | ||
| case "json": | ||
| return JSON | ||
| case "elvish": | ||
| return Elvish | ||
| case "tcsh": | ||
| return Tcsh | ||
| case "vim": | ||
| return Vim | ||
| case "zsh": | ||
| return Zsh | ||
| } |
There was a problem hiding this comment.
I hope this is alright, sorting sortable things is something I do out of habit. It does tend to be nice when looking things up, but can very much drop this commit if you don't want the noise.
|
It seems to me that the current test setup is geared heavily for interactive use of direnv via the shell hook/eval setup. This doesn't really make sense for the gha format unless I should have a gha -> bash converter and then run the common tests? If not, then I'd like to know which tests I should implement? I would think maybe just comparing exported envs as redirected into a file? |
mmlb
force-pushed
the
export-github-actions
branch
2 times, most recently
from
2e100ea
to
d488237
Compare
| @@ -42,3 +42,18 @@ jobs: | |||
| GO111MODULE: on | |||
| run: make test-stdlib test-bash | |||
|
|
|||
| - name: GitHub Actions Env Test Setup | |||
There was a problem hiding this comment.
I was going to create a separate workflow file for this test but decided to just append the tests here instead. I can split it to different files if that makes more sense.
Should I "port" this test to do something when running locally on a dev machine? It feels kind of odd to only test some functionality in CI vs locally first.
|
Hey @zimbatm gentle ping. Hopefully not causing noise for you but I'm not sure if you've seen this PR. |
|
I've tested this in a self-hosted Ubuntu runner, and it works as expected 👍 |
|
Looks good to me. @mmlb do you mind checking why the CI is failing? Other than that, I'm happy with the changes. |
|
Hey @zimbatm I'm not sure why CI failed, I've added a debug commit to see if something pops up can you please release it for testing? edit: |
mmlb
force-pushed
the
export-github-actions
branch
4 times, most recently
from
eb4dfe8
to
73aa8e5
Compare
|
Test failure might be due to $SRANDOM only existing in bash >5.1! |
mmlb
force-pushed
the
export-github-actions
branch
6 times, most recently
from
a6759b0
to
a75d22d
Compare
|
Hey @zimbatm this should be ready to go now, pretty sure I've hammered out the CI failures. PTAL. edit: oops I missed that windows is failing, going to try and figure it out but my windows scripting knowledge is non-existent. |
mmlb
force-pushed
the
export-github-actions
branch
4 times, most recently
from
afd20e5
to
9df33b9
Compare
|
ok @zimbatm I've punted on windows like the |
.github/workflows/nix.yml
Outdated
| branches: [ master ] | ||
| pull_request: {} |
There was a problem hiding this comment.
This change was just for testing and needs to be restored
There was a problem hiding this comment.
Woops yes indeed. There's a similar change on go.yml, I presume that was is ok to keep?
There was a problem hiding this comment.
Both should use:
on:
push:
branches: [ master ]
pull_request: {}
Once that's restore the PR is ready to merge :)
There was a problem hiding this comment.
Done!
I'm curious why you want to limit the branches to just master though? Running the tests for all branches makes for an easier time for first time contributors while GH gates the CI builds. Though I guess those contributors could just PR off of master in their fork, but that doesn't seem to be the norm for most people.
There was a problem hiding this comment.
I kinda settled on this as a middle ground. As a maintainer, I want to have PRs be tested, and adding push on top adds more noise as both will be running then.
GHA has both simple and multi-line env var modes, I decided to always use the multiline mode for ease of implementation.
Only doing this in CI because it doesn't seem worth it to mock GHA to test this, especially conserding `act` already does a good job of mocking GHA.
mmlb
force-pushed
the
export-github-actions
branch
from
9df33b9
to
364e07a
Compare
|
This doesn't seem to work well with modifications to I am trying an approach like this: - name: Load PATH changes
run: |
direnv_path_value="$(direnv exec . sh -c 'echo $PATH')"
IFS=: # loop over PATH entries, which are separated by ":"
for direnv_path_entry in $(echo "$direnv_path_value"); do
if ! echo "$PATH" | grep "$direnv_path_entry" &> /dev/null; then
echo "GITHUB_PATH does not include $direnv_path_entry, adding now"
echo "$direnv_path_entry" >> "$GITHUB_PATH"
fi
done
- name: Load other environment changes
run: direnv export gha >> "$GITHUB_ENV"If I'm right, I could write a failing test to prove it, and maaaaybe add a |
|
if I understand what you mean properly, the step could be simplified to: - name: Load PATH changes
run: direnv exec . sh -c 'echo $PATH' > "$GITHUB_PATH"
- name: Load other environment changes
run: direnv export gha >> "$GITHUB_ENV" |
|
Related issue: actions/toolkit#655 |
I've added a GitHub Actions export only shell as I can't really see how a hook for it would be used or wanted.
Fixes #903