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

Factor out tar-artifacts action #148

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Factor out tar-artifacts action #148

merged 3 commits into from
Oct 24, 2024

Conversation

theihor
Copy link
Contributor

@theihor theihor commented Oct 21, 2024

Rework tar-artifact.sh script, making it suitable for use as an independent github action.

Use tar -rf (append) to accumulate files of interest before compressing with zstd.

Control groups of artifacts to include (e.g. selftests/bpf, selftests/sched_ext) with environment variables.

@theihor theihor self-assigned this Oct 21, 2024
@theihor theihor force-pushed the tar-artifacts branch 4 times, most recently from c9ad352 to 0858df1 Compare October 21, 2024 22:11
Rework tar-artifact.sh script, making it suitable for use as an
independent github action.

Use tar -rf (append) to accumulate files of interest before
compressing with zstd.

Control groups of artifacts to include (e.g. selftests/bpf,
selftests/sched_ext) with environment variables.
@theihor theihor marked this pull request as ready for review October 21, 2024 23:00
@theihor theihor requested a review from chantra October 21, 2024 23:00
theihor added a commit to theihor/vmtest that referenced this pull request Oct 21, 2024
Copy link
Collaborator

@chantra chantra left a comment

Choose a reason for hiding this comment

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

tar -r trick is nice to separate each step. I suppose this could increase IO compare to tar | zstd, but at the same time, we used to do some copies left and right before, so probably on par with what we had.

uses: ./tar-artifacts
env:
ARCHIVE_BPF_SELFTESTS: 'true'
ARCHIVE_MAKE_HELPERS: ${{ github.repository != 'kernel-patches/bpf' && 'true' || '' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought for later: maybe we should move this out of libbpf/ci eventually and let the caller set it through an env var or input (probably preferred)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this is why I made them env vars in the first place. They are set in the workflow step right now, I guess you're talking about a case when the workflow itself is called from outside.

When we figure out how to actually use reusable workflows in kernel-patches/vmtest, this might be worth revisiting.

Comment on lines 11 to 16
toolchain:
required: true
outputs:
archive-name:
description: 'Artifacts tarball name'
value: ${{ steps.run-script.outputs.archive-name }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

toolchain is only needed so we can generate the final archive name. I wonder if we should rather provide the destination file as an input, this way we can give it the name we like from the caller.

It also seems to be what other actions doing something similar would do: https://github.com/marketplace/actions/tar-action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added archive input and removed output.

I thought about this, and wasn't sure what's the best API would be. The caller obviously needs to know the archive name, so why don't just allow them to set it.

Also, it seems so far that using env variables to control action-driven scripts is very convenient. We should think about a convention for when to use env variables vs script argument vs action inputs, because right now it's quite chaotic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

because right now it's quite chaotic.

could not agree more :D

* Remove `archive-name` output
* Remove `toolchain` input
* Add `archive` input, setting the path to output .zst archive
* Set ARCH env variable instead of passing a script argument
@theihor theihor requested a review from chantra October 23, 2024 18:41
theihor added a commit to theihor/vmtest that referenced this pull request Oct 23, 2024
theihor added a commit to theihor/vmtest that referenced this pull request Oct 23, 2024
@theihor theihor merged commit 4c85516 into libbpf:main Oct 24, 2024
14 checks passed
theihor added a commit to theihor/vmtest that referenced this pull request Oct 24, 2024
theihor added a commit to theihor/vmtest that referenced this pull request Oct 24, 2024
theihor added a commit to kernel-patches/vmtest that referenced this pull request Oct 24, 2024
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