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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
eb4dfe8
to
73aa8e5
Compare
Test failure might be due to $SRANDOM only existing in bash >5.1! |
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was just for testing and needs to be restored
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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