Add deploy-lambdas reusable workflow #5
Conversation
| name: Deploy Staging Lambdas | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: timbru31/node-alpine-git |
There was a problem hiding this comment.
I assume that you chose timbru31/node-alpine-git because it based on the official node alpine image with the benefit of having git installed. Do you think that the comfort eliminating the apk install git is worth it compared to using the official node image which offers more version and is more up to date (as a consequence more secure under some circumstances)?
There was a problem hiding this comment.
Really I just copied it from the other workflow. @thejamespower made the change I believe - what are your thoughts?
There was a problem hiding this comment.
It's probably better to use the official one and use apk to add git, I was just being lazy
| cache: npm | ||
| - name: 'NPM: Add Config and Authorization' | ||
| run: | | ||
| rm -f .npmrc |
There was a problem hiding this comment.
The .npmrc file might not exist which would cause rm to exit with a non-zero exit code. Also redirecting the echo command output below, using >, will surely override the contents of .npmrc.
There was a problem hiding this comment.
AFAIK the -f flag means that if it doesn't exist it won't error. Also as I remember I had found that > wasn't overriding the original file... quite likely I made some sort of mistake though
There was a problem hiding this comment.
You're right about the -f flag. Although it can also be used for ignoring confirmation prompts and similar. The redirection should work on the other hand. One possibility is that the original .npmrc file is protected and as such cannot be overriden, thus must be forcefully removed and recreated. Though I don't think that's likely.
| npm ci | ||
| npm run build | ||
| rm -rf node_modules | ||
| npm ci --only=production --ignore-scripts |
There was a problem hiding this comment.
The --only-production option is not referenced in the documentation. Would you go along with removing it?
There was a problem hiding this comment.
yeah looks like npm prune --production is a better option for what they're trying to do
There was a problem hiding this comment.
Interesting remark. In summary, since NODE_ENV is not set to production, npm ci will install devDepedencies which are required to build. Afterwards, they can be removed via npm prune --production. Consequently, rm -rf node_modules is redundant.
There was a problem hiding this comment.
Additionally, in case that all depdencies, including devDependencies are installed, running cross-env NODE_ENV=production npm ci will result in the removal of devDependencies. However, semantically spreaking, the prune command makes much more sense.
On the other hand, considering that npm ci will override versions based on the current values specfied in package.json and package-lock.json, the first rm -rf node_modules command is redundant as well.
There was a problem hiding this comment.
npm prune didn't work as I expected and we were left with a massive zip
.github/workflows/deploy-lambdas.yml
Outdated
| # semantic_version: 19 | ||
| # - name: Get previous tag | ||
| # id: previoustag | ||
| # uses: 'WyriHaximus/github-action-get-previous-tag@v1' |
There was a problem hiding this comment.
Since the section is commented out, I assume that your intention might be to remove it. Although, if you decide to keep it, pray consider using git describe --tags $(git rev-list --tags --max-count=1) to get the latest tag (reference).
There was a problem hiding this comment.
It's commented out for now as we were trying to get James' branch deployed and there were various issues interfering
richardtbell
force-pushed
the
EHP-232-migrate-lambda-workflows-to-workflows-repo
branch
2 times, most recently
from
b49b77c to
62240f6
Compare
mateja176
force-pushed
the
main
branch
6 times, most recently
from
9c8b186 to
129b86f
Compare
No description provided.