-
Notifications
You must be signed in to change notification settings - Fork 86
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
GHA workflow #360
GHA workflow #360
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.
Great progress! I see a few important differences compared to the CCI config.
.github/workflows/update-i18n.yml
Outdated
@@ -0,0 +1,39 @@ | |||
name: Update i18n |
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.
This repo doesn't need this workflow since it doesn't have any localized UI
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.
Got it. Thnx!
@@ -12,3 +12,6 @@ dist/* | |||
# Editors | |||
/#* | |||
*~ | |||
|
|||
# Act | |||
.secrets |
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 idea! I'll add this in some of my work...
.github/workflows/ci-cd.yml
Outdated
export VERSION=${VPKG}-prerelease.${RELEASE_TIMESTAMP} | ||
|
||
echo "RELEASE_VERSION=${VERSION}" >> $GITHUB_ENV | ||
echo "NPM_TAG=latest" >> $GITHUB_ENV |
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.
Looks like this is missing the hotfix handling from the CircleCI config:
scratch-svg-renderer/.circleci/config.yml
Lines 42 to 59 in 0705358
name: Setup Deploy | |
command: | | |
export NODE_ENV=production | |
export RELEASE_TIMESTAMP=$(date +'%Y%m%d%H%M%S') | |
echo "export NPM_TAG=latest" >> $BASH_ENV | |
npm run build | |
if [ -z "$BEFORE_DEPLOY_RAN" ]; then | |
VPKG=$($(npm bin)/json -f package.json version) | |
VERSION=${VPKG}-prerelease.${RELEASE_TIMESTAMP} | |
echo "export RELEASE_VERSION=${VERSION}" >> $BASH_ENV | |
npm --no-git-tag-version version $VERSION | |
if [[ "$CIRCLE_BRANCH" == hotfix/* ]]; then # double brackets are important for matching the wildcard | |
echo "export NPM_TAG=hotfix" >> $BASH_ENV | |
fi | |
git config --global user.email "$(git log --pretty=format:"%ae" -n1)" | |
git config --global user.name "$(git log --pretty=format:"%an" -n1)" | |
echo "export BEFORE_DEPLOY_RAN=true" >> $BASH_ENV | |
fi |
Sorry this is so inconsistent between repositories, but this repo in particular has been a source of hotfixes in the past so I think it's important to retain that support
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.
Thanks, yeah I think I ignored it for convenience :-D
on it!
package-lock.json
Outdated
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.
Thanks for moving to npm ci
and adding a package lockfile! 😎
Oh, right... looks like the commitlint step is complaining about the commit messages not following conventional commits. Most of the front-end & editor repositories use that format now. |
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.
Sorry, I had this review pending and forgot to submit it.
This looks like it'll work! Thanks :)
.github/workflows/ci-cd.yml
Outdated
push: # Runs whenever a commit is pushed to the repository | ||
branches: [main, develop, hotfix/*] |
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.
Ah, that makes sense. Thanks!
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.
The branch is actually called "master" here.
00210d7
to
98ed1d5
Compare
I used @cwillisf GHA template for scratch-vm as a basis.
Additions/changes:
That last bit wasn't tested, because npm publish has a --dry-run flag which can be removed once this code is approved. If the tagging then fails I need to look at that again.fixed with a condition.