Skip to content
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

refactor: minio -> s3 #23

Merged
merged 11 commits into from Mar 26, 2024
Merged

refactor: minio -> s3 #23

merged 11 commits into from Mar 26, 2024

Conversation

oprypkhantc
Copy link
Contributor

Closes #21

@oprypkhantc oprypkhantc marked this pull request as draft March 26, 2024 00:23
Copy link
Collaborator

@DrJume DrJume left a comment

Choose a reason for hiding this comment

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

Looking good so far. Thanks 👍

docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
@LouisHaftmann LouisHaftmann self-requested a review March 26, 2024 08:46
@oprypkhantc
Copy link
Contributor Author

@DrJume @LouisHaftmann Added defaults for the s3 driver that point to AWS S3. The title of the docs page now includes both S3 and MinIO, and the verbiage is now S3 storage to make it more clear the driver works with any S3-compatible storage

@oprypkhantc oprypkhantc marked this pull request as ready for review March 26, 2024 12:20
Copy link
Collaborator

@LouisHaftmann LouisHaftmann left a comment

Choose a reason for hiding this comment

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

We should probably test whether the prune function actually works with s3/minio. It is currently only used for unit tests so it's not that important.

docs/content/index.yml Outdated Show resolved Hide resolved
@LouisHaftmann
Copy link
Collaborator

only one small change. after that we can merge this pr and I'll create a new pre-release build.

Thanks for your work so far! ❤️

@oprypkhantc
Copy link
Contributor Author

Done. I've made sure to test the pruning - it works with S3 as expected :)

@LouisHaftmann LouisHaftmann linked an issue Mar 26, 2024 that may be closed by this pull request
storage-drivers/s3.ts Outdated Show resolved Hide resolved
storage-drivers/s3.ts Outdated Show resolved Hide resolved
storage-drivers/s3.ts Outdated Show resolved Hide resolved
storage-drivers/s3.ts Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Outdated Show resolved Hide resolved
docs/content/2.storage-drivers/s3.md Show resolved Hide resolved
oprypkhantc and others added 2 commits March 26, 2024 18:02
Co-authored-by: Julian Meinking <12785972+DrJume@users.noreply.github.com>
@DrJume DrJume merged commit aac8659 into falcondev-oss:dev Mar 26, 2024
@oprypkhantc
Copy link
Contributor Author

By the way, have you thought about making an issue/PR to actions/runner to allow setting a custom ACTIONS_CACHE_URL without patching the runner? We're not running them in Docker (as this has it's own challenges with Docker-in-Docker setup), meaning we had to disable automatic self-updates for the patch to stay. So maybe there's a change GitHub can support this natively 🙃

@oprypkhantc oprypkhantc deleted the minio-with-s3 branch March 26, 2024 16:09
@DrJume
Copy link
Collaborator

DrJume commented Mar 26, 2024

@DrJume
Copy link
Collaborator

DrJume commented Mar 26, 2024

By the way, have you thought about making an issue/PR to actions/runner to allow setting a custom ACTIONS_CACHE_URL without patching the runner? We're not running them in Docker (as this has it's own challenges with Docker-in-Docker setup), meaning we had to disable automatic self-updates for the patch to stay. So maybe there's a change GitHub can support this natively 🙃

Yeah, self-updates would be nice. We didn't think about that, because we use ephemeral runners, but it's a valid use case.
Maybe it is possible to hook into the self-update with a script which is executed afterwards to re-patch the binary?

@oprypkhantc
Copy link
Contributor Author

Yeah this would be nice, but sadly it seems that GitHub is not interested in this change.

😢

Yeah, self-updates would be nice. We didn't think about that, because we use ephemeral runners, but it's a valid use case.
Maybe it is possible to hook into the self-update with a script which is executed afterwards to re-patch the binary?

Not sure, our devops manage that. We'll have to do something like that as GitHub does not promise compatibility with older version runners 😞 :

If you do not perform a software update within 30 days, the GitHub Actions service will not queue jobs to your runner. In addition, if a critical security update is required, the GitHub Actions service will not queue jobs to your runner until it has been updated.

@oprypkhantc
Copy link
Contributor Author

@DrJume So it turns out it's almost impossible to make it work with non-ephemeral runners :/

Looking through the runner's code, there are no hooks we could use. Most of the self-update stuff is done on the C# side and there's a single sh script that runs to finalize the update, but the problem is that it's copied from the newly downloaded binary as opposed to just root of the runner. Moreover, there's no command to run the self-update - the only way it can be triggered is through GitHub sending specific messages to the runner.

That's unfortunate because it looks like all we can do is also run ephemeral runners :(

@DrJume
Copy link
Collaborator

DrJume commented Apr 1, 2024

@DrJume So it turns out it's almost impossible to make it work with non-ephemeral runners :/

Looking through the runner's code, there are no hooks we could use. Most of the self-update stuff is done on the C# side and there's a single sh script that runs to finalize the update, but the problem is that it's copied from the newly downloaded binary as opposed to just root of the runner. Moreover, there's no command to run the self-update - the only way it can be triggered is through GitHub sending specific messages to the runner.

That's unfortunate because it looks like all we can do is also run ephemeral runners :(

A last option maybe could be to watch the runner binary and patch it automatically.
I would recommend https://github.com/parcel-bundler/watcher for file watching with NodeJS.

But still, we would also recommend to use ephemeral runners.

@LouisHaftmann
Copy link
Collaborator

we are using ephemeral runners in our setup but kubernetes is required for docker in docker to work. maybe we could put together a guide for you if you're interested

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.

Support for S3
3 participants