Re-implement Action in TypeScript #45
Conversation
pbrisbin
force-pushed
the
pb/typescript
branch
from
1c60f4a to
4fbed09
Compare
| @@ -0,0 +1,22 @@ | |||
| { | |||
| "$schema": "https://json.schemastore.org/tsconfig", | |||
There was a problem hiding this comment.
Handy to define -- is there any tool in particular that motivated this?
There was a problem hiding this comment.
Most everything boilerplate here was copied from grep-action, which originally used our typescript-action-template repo and has since grabbed some stuff from this official template. 🤷
|
🎉 this looks ready to go
Let me know if anyone disagrees with (1). |
|
Heh, welp, I found one issue: stack-build-arguments: |
--ghc-options "$STACK_GHC_OPTIONS"This no longer works, because the value is not passed to a shell, it's given to
|
|
I did (3) 🤷 |
There was a problem hiding this comment.
envsubst.ts seems like it has the necessary test cases. The third-party library I found for this (please no leftpad jokes) is a little more complicated but doesn't seem to cover any particular use-case that is missing here.
I might find it surprising that an action implemented its own env substitution, though, and would expect an underlying sh instead. I understand why that more complicated approach would have a bug risk, but could you elaborate on the impact to build logs?
Despite that question, I'm fine with this approach as-is.
It's minor, but right now the build logs may show: % /usr/bin/stack build --test
stack logs...Or % /usr/bin/stack build --test
Error: /usr/bin/stack exited 1And now they would show, % /bin/sh -c /usr/bin/stack build --test
stack logs...And % /bin/sh /usr/bin/stack build --test
Error: /bin/sh exited 1Which is just a little potentially confusing if you were expecting |
We generally prefer TypeScript for actions, and since this is almost entirely doing things available as libraries (`@actions/cache` and `@actions/exec`), it will be easy to port. Once in TypeScript, we can continue to extend and fix in a more comfortable language.
- Fix `cacheSaveAlways` behavior - Prefer positive conditional - Use `jest.spyOn` - Encapsulate `getStackDirectories` - Parse and cache stack programs too - Track `--resolver` and use that instead of yaml resolver in cache key - Don't test so many resolvers - Remove `nightly` examples, we unit test that now - Remove explicit resolver cache prefixes, we fixed that in the action - Fix reading `LOCALAPPDATA` - Replace environment variables in shellwords inputs
pbrisbin
force-pushed
the
pb/typescript
branch
from
be58608 to
3bdf997
Compare
|
FYI I restored the branch for now, until I update our test repositories to go back to |
We generally prefer TypeScript for actions, and given that this action is almost entirely just using
@actions/cacheand invokingstack(which can be done better with@actions/exec), it makes sense to use TypeScript even though it's "simple".This brings the following benefits to users:
We can now properly group our output and log details such that the workflows are much easier to debug (e.g. for caching behaviors).
@actions/execalso makes command invocations logged better, incorporating rerun-with-debug, etc.Some of the changed behaviors (described below) are bug-fixes
This brings the following benefits to maintainers:
stack pathas outputs without having to maintain the list inaction.ymlThe following behaviors were changed along the way:
We no longer skip the Build step on a primary-key hit
This is still (I think) fine (and preferred) to do on the Dependencies step, since there are no artifacts useful outside of the build produced in that step. However, if you are relying on the Build step to produce executables (for example), skipping it on a cache-hit means no executable and that will fail.
We are very careful to not install GHC pre-caching
See Don't install ghc before restoring dependencies #33. There are some caveats being discussed there, and this PR implements what's described.
We run
stack upgradebefore getting startedAnd added a
no-stack-upgradeinput to disable that if desired. Note that Stack v2.15 (newer than currently on the runners) is required to address the above, which is why I added this.