Skip to content

Conversation

@kevinschoonover
Copy link
Contributor

@kevinschoonover kevinschoonover commented Mar 28, 2022

Hey Folks!

I was working on this tangentially for https://github.com/bloominlabs/setup-earthly/ and I didn't realize this was being made from the current docs!

I took a lot of inspiration (i.e. a lot of the source code) from the setup-pulumi and this gives the action:

  1. typescript integration
  2. caching via the tool-cache
  3. tests
  4. macOS + windows support

and was wondering if you were interested in upstreaming the code?

@kevinschoonover kevinschoonover changed the title initial commit update to use typescript + tool-cache Mar 28, 2022
@kevinschoonover
Copy link
Contributor Author

@alexcb do you have any cycles to take a peek at this?

Copy link
Contributor

@alexcb alexcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR -- it's a rather big change -- switching languages.

if src/setup-earthly.js is no longer needed, can it be deleted?

Can you move the tests into our Earthfile -- so we can use earthly to test it rather than act.

@kevinschoonover
Copy link
Contributor Author

kevinschoonover commented Apr 19, 2022

@alexcb The latest commit should address all of your concerns except the last one. I added a earthly +test to run the unit tests; however, I'm not sure how to move all of the integ tests at .github/workflows/test.yml into earthly itself since we want to test how github actions would exercise the code which im not sure we can do faithfully inside of earthly itself (unless we ran act instead earthly which would be a dind thing)

I can try adding something like

latest:
  name: Test Latest Version Install
  strategy:
    matrix:
      platform: [ubuntu-latest, macos-latest, windows-latest]
  runs-on: ${{ matrix.platform }}
  steps:
    - uses: actions/checkout@v2
    - uses: actions/setup-earthly@v1 # < installs earthly
    - run: earthly +compile
    - uses: ./ # < installs earthly again
    - run: earthly --version

but the test will still be having github actions import the build file and run it by itself which is what will happen with consumers of the action (i.e. what we should test).

We also have a bootstrapping problem where we will always test the previous released version of the action instead of the current one which you can see in the previous example if we wanted to test actions/setup-earthly@v2 for instance.

Let me know if those comments / concerns make sense.

@alexcb
Copy link
Contributor

alexcb commented Apr 20, 2022

Thanks for the updates @kevinschoonover

I'm not sure how to move all of the integ tests at .github/workflows/test.yml into earthly itself since we want to test how github actions would exercise the code which im not sure we can do faithfully inside of earthly itself

I tried adding a new earthly target:

test-run:
    FROM +npm-base
    COPY --dir +compile/dist .
    ENV RUNNER_TOOL_CACHE=/tmp/cache-dir
    RUN node dist/index.js
    RUN earthly --version # this currently fails due to earthly not being installed on the path

with the hopes of running the actions code directly in earthly, which should result in installing earthly-in-earthly. But I quickly discovered earthly is no longer installed to the same location (and as a result isn't on my cache).

I've never used tool-cache before, but I quickly discovered all I need to do is set ENV RUNNER_TOOL_CACHE=/tmp/cache-dir.

Perhaps this is where we can run act inside earthly under a WITH DOCKER command, rather than setting these ENV values by hand; however for the sake of curiosity (and simplicity), I wanted to see if I could run it directly:

node dist/index.js

I see

Configured range: 
Matched version: v0.6.14
Install destination is /root/.earthly
Successfully deleted pre-existing /root/.earthly/bin
::debug::downloading https://github.com/earthly/earthly/releases/download/v0.6.14/earthly-linux-amd64
::debug::Downloading https://github.com/earthly/earthly/releases/download/v0.6.14/earthly-linux-amd64
::debug::Destination /root/.earthly/bin/earthly
::debug::download complete
::debug::successfully downloaded https://github.com/earthly/earthly/releases/download/v0.6.14/earthly-linux-amd64 to /root/.earthly/bin/earthly
::debug::Caching tool earthly 0.6.14 x64
::debug::source dir: /root/.earthly/bin
::debug::destination /tmp/cache-dir/earthly/0.6.14/x64
::debug::finished caching tool
::add-path::/tmp/cache-dir/earthly/0.6.14/x64

and I can verify the file was downloaded and cached to:

ls -la /tmp/cache-dir/earthly/0.6.14/x64/earthly
-rwxr-xr-x    1 root     root      33436456 Apr 20 19:34 /tmp/cache-dir/earthly/0.6.14/x64/earthly

However, when I re-run node dist/index.js, I get the same Downloading message, and then when I looked at the cached file:

/ # ls -la /tmp/cache-dir/earthly/0.6.14/x64/earthly
-rwxr-xr-x    1 root     root      33436456 Apr 20 19:35 /tmp/cache-dir/earthly/0.6.14/x64/earthly

The time value has changed (from 19:34 to 19:35).

This makes it look like the file isn't being cached correctly. Can you elaborate on the expected behaviour of tool-cache?

@kevinschoonover
Copy link
Contributor Author

kevinschoonover commented Apr 20, 2022

I did some digging into this a couple days ago because I was experiencing the same behavior. It seems tool-cache is for self hosted runners. There is a different sdk that does the caching we expect.

I have a branch on my fork called 'caching' which implements real caching. Dont know if we want to include that in this PR or follow up. However it'll add a wrinkle to the running it directly in earthly because it requires special environment variables and running another .js file.

I can look into what you're mentioning above tonight

@alexcb
Copy link
Contributor

alexcb commented Apr 20, 2022

https://www.npmjs.com/package/@actions/tool-cache makes reference to a tc.find -- perhaps that's the missing piece required to re-use the cache? That might be easier than swapping it out for a different sdk.

@kevinschoonover
Copy link
Contributor Author

Happy to implement this, but each github action runner is on a fresh install so I don't think we will see any benefit from it except on local/self hosted runs.

@kevinschoonover
Copy link
Contributor Author

kevinschoonover commented Apr 21, 2022

@alexcb I added both toolcache and the github action caching so we should now effectively have 2-tiers of cache:

  1. toolcache - locally on disk which is best for self-hosted runners
  2. 'regular cache' - effectively the @action/cache which is best for GH runners since it'll download from their caching service
  3. downloading regularlly

On your point about moving on the integration tests from .github/workflows/test.yml, are we getting a lot of benefit from not having github action test itself by importing and running the action? For reference I'm talking about the lines

https://github.com/earthly/actions-setup/pull/2/files#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88R24-R94

which seem to be a pretty standard practice for testing actions:

  1. https://github.com/pulumi/setup-pulumi/blob/35d1876b6cf7fad106567efd1600db81bc55d8fe/.github/workflows/test.yml#L23-L88
  2. https://github.com/actions/setup-node/blob/main/.github/workflows/e2e-cache.yml
  3. https://github.com/actions/cache/blob/8f1e2e02865c42348f9baddbbaafb1841dce610a/.github/workflows/workflow.yml#L52-L143

the problem with dind + act is that act doesn't support all of githubs features like caching and my not reflect what the actual github runner will do for the various platforms (linux, macos, windows). Moreover, I think the problem you're having with earthly not being in the path is because the actions SDK emits a command that is picked up by the runner to actually change the system path and only actually affects the process's path.

I'm happy to add an extra test to earthly which will compile and run the code like you were talking about above in addition to these tests if thats a good compromise?

Copy link
Contributor

@alexcb alexcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the caching work -- I did a few tests and it appears to be working.

@alexcb alexcb merged commit 6e1af8d into earthly:main Apr 21, 2022
@alexcb
Copy link
Contributor

alexcb commented Apr 21, 2022

I'm happy to add an extra test to earthly which will compile and run the code

I'll handle this. Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants