-
Notifications
You must be signed in to change notification settings - Fork 49
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
Refactoring CI pipeline in preparation for documenting it #1152
Conversation
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.
Nice
.github/workflows/build.yml
Outdated
@@ -111,7 +89,7 @@ jobs: | |||
else | |||
export PUBLISH=true | |||
fi | |||
echo "::set-env name=PUBLISH::${PUBLISH}" | |||
export PUBLISH=true |
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.
i know this is WIP but still
@@ -134,10 +112,6 @@ jobs: | |||
path: node_modules | |||
key: ${{ runner.os }}-${{ hashFiles('yarn.lock') }}-yarn | |||
|
|||
- name: Building NodeJS dependencies - host OS |
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.
probably ditto?
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.
actually we put both dependence updates in the same script
.github/workflows/build.yml
Outdated
@@ -154,61 +128,45 @@ jobs: | |||
run: echo "::set-env name=BRANCH::${GIT_BRANCH}" | |||
|
|||
- name: Security audit Node modules | |||
run: yarn run security-audit | |||
run: ./scripts/security-audit.sh |
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 create scripts for one liners?
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.
Longer discussion, let's have it "live" ;)
scripts/license-audit.sh
Outdated
|
||
source $DIR/_common.sh | ||
|
||
cd $PROJECT_ROOT |
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.
suggest doing this in subshell
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.
👍🏻
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.
👍🏻
scripts/prepare_deps.sh
Outdated
@@ -7,6 +7,10 @@ source $DIR/_common.sh | |||
|
|||
# Build the node_modules as well as the base image for the final outputs and store it in the shared cache so it can be reused later | |||
|
|||
cd $PROJECT_ROOT |
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.
same, subshell
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.
👍🏻
@@ -148,67 +123,51 @@ jobs: | |||
|
|||
- name: Building NodeJS dependencies | |||
if: steps.cache-deps.outputs.cache-hit != 'true' |
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.
Only thing I can see is that we might want to check the cache-hit status of steps.node-modules.outputs.cache-hit
as well - I know it depends on the same file but if one was invalidated somehow or something... /shrug
Supports #389
What
Pull parts of the GHA in scripts that can be run locally
Why
See https://github.com/island-is/handbook/blob/master/docs/adr/0002-continuous-integration.md#decision-drivers-policy
Checklist: