-
Notifications
You must be signed in to change notification settings - Fork 0
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
Modular deployments #29
Conversation
Build is run before tests anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all: well done! I like the refactoring into reusable workflows and the code de-duplication.
About the edge case regarding concurrency: I'm not sure this really is an edge case. Imagine re-naming a shared component or adding a property to a shared components. It's a simple and not too unusual change which would already trigger the case and might lead to problems.
Another minor thing I would like to have improved is the code duplication between the pipeline-<package>.yml
s and the need to duplicate this workflow for all new apps.
I have a solution in mind which I didn't try out yet but which might solve both of the issues above:
Instead of having multiple pipeline-*
workflows triggered depending on on.push.paths
, we only keep the shared pipeline. This pipeline implements a similar functionality as on.push.paths
to check which paths have been touched and runs the jobs for all packages depending on the same paths
patterns (e.g. using the paths-filter GHA). By doing that we solve the concurrency problem since there won't be multiple workflows trying to deploy the same application and there is no code duplication or explicit workflows for apps since all apps share the same pipeline.
.github/workflows/pipeline-dito.yml
Outdated
check-and-test: | ||
uses: ./.github/workflows/check-and-test.yml | ||
with: | ||
flag: "--workspace=packages/dito" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the input the whole flag instead of just the package name (similar to the build-and-deploy.yml
workflow? Are there current use cases where this is needed? Otherwise I would say YAGNI and KISS 🤪 and streamline aka. use only package name as input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finde die Varianten alle nicht besonders schön: https://github.com/orgs/community/discussions/25725
This reverts commit 41995ff.
This reverts commit 2c0aff8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job!
This PR introduces several changes to make the pipeline modular and dependent on which parts of the code were changed:
Preparatory changes
test
andaudit-licenses
into singular reusablecheck-and-test.yml
workflowscan.yml
instead of recreating the steps invulnerability-scan
Modularity
check-and-test
with matrix of changed packages to only run tests of changed packagesbuild-and-deploy
with matrix of changed packages and setmax-parallel
to 1 to prevent git push conflictsdeploy
step to only deploy a package once concurrentlyREADME
with instructions on how to create new pipelineArchive
Modularity
pipeline-shared.yml
check-and-test.yml
andscan.yml
build-and-deploy.yml
using a matrix strategy for every apppipeline-<app>.yml
is runcheck-and-test.yml
with a--workspace=packages/<app>
flag for the applicable npm actionsscan.yml
build-and-deploy.yml
for the respective app onlyREADME
with instructions on how to create new pipelineWhen pushing multiple commits containing changes to the shared code and multiple apps this could lead to runs being canceled wrongly and leading to apps not being deployed correctly:
app1
app2
as deployment ofapp1
is still runningapp2
first, cancelling the first deployment ofapp2
app1
, cancelling its own deployment ofapp2
I think this is a minor edge case but something we should look out for.