run makefile tests for every sample in a separate CI workflow #247
Conversation
HarshCasper
force-pushed
the
pro-samples-ci
branch
from
5da104a to
9925caa
Compare
There was a problem hiding this comment.
if we are commenting the whole file out, why not just remove it?
There was a problem hiding this comment.
Ah, that would be removed once I have an explicit approval that this is the way to go forward 😅
There was a problem hiding this comment.
In my opinion, with git being used properly and everywhere, there is never a need for commented out code. You can always jump back, and commented code can't make it into code reviews or even in the mainline (where it just causes confusion and maintenance overhead - also for example like these comments here 😛).
There was a problem hiding this comment.
In my opinion, with git being used properly and everywhere, there is never a need for commented out code. You can always jump back, and commented code can't make it into code reviews or even in the mainline (where it just causes confusion and maintenance overhead - also for example like these comments here 😛).
| prepare_list: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - id: set-matrix | ||
| run: | | ||
| echo "matrix=[$(find . -mindepth 2 -type f -name 'Makefile' ! -path './sample-archive/*' | sed 's|/Makefile||' | awk '{printf "\"%s\",", $1}' | sed 's/,$//')]" >> $GITHUB_OUTPUT | ||
| - run: echo "matrix=[$(find . -mindepth 2 -type f -name 'Makefile' ! -path './sample-archive/*' | sed 's|/Makefile||' | awk '{printf "\"%s\",", $1}' | sed 's/,$//')]" |
There was a problem hiding this comment.
This is a pretty dangerous approach, because it does not limit the amount of parallel runners at all. GitHub organizations have a limit of 20 concurrent Linux runners for free in public repos: https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration
This means that it would basically block all GitHub action executions for the time it takes to execute the pipeline for the whole localstack-samples org.
Since these samples are executed somewhat fast, it can be a conscious decision to leave it like this, but I would propose to at least limit the amount of concurrent runners to less than 20 (maybe 5-10) to allow other pipelines in the org to be scheduled at the same time.
You can implement that super-easily with the max-parallel setting in the matrix config: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#defining-the-maximum-number-of-concurrent-jobs
No description provided.