Skip to content
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

build: Update CI/CD workflows #1428

Closed
wants to merge 23 commits into from

Conversation

ankurdotb
Copy link
Contributor

@ankurdotb ankurdotb commented Apr 12, 2023

This PR doesn't change any code: it primarily focusses on improving the workflows used for CI/CD in this repository. A few elements can be quickly discussed and resolved, but until then it's NOT to be merged. (I'm not setting to "draft" as that disables workflow runs).

Outstanding questions

  • Should job runners be consistently set to ubuntu-20.04 or ubuntu-22.04? (ubuntu-latest now points to ubuntu-22.04. There was a transition period when it could have pointed to either near end of 2022.)
  • Base image for Dockerfile (same as above)
  • Keep local copy of setup-node Action or ditch it? (Explained below)
  • actions/setup-libssl seems to be adding a package designed for Ubuntu 18.04. Ideally, this should be changed to the package for Ubuntu 20.04 or Ubuntu 22.04 (depending on runner decision above).
  • Use semantic-release to auto-calculate version numbers and whether to release? (See releaserc.json definition in @cheqd/sdk for an example). This uses a custom/default Convention Commit convention to auto-trigger releases.
  • Proper multi-stage Docker builds

Changelog

Continuous Integration/Deployment workflows

  • I see there's a local copy of action/setup-node which is used throughout the workflows. It's not strictly necessary including the caching directives, as the default cache handling option itself automatically does this. Using the actual upstream actions/setup-node@v3 ensures all upstream security updates are used in the workflow. I'm not too fussed whether a local copy is kept, but should ideally be bumped to v3 and can be simplified to use the built-in cache handling. (v2 required manual configuration.)
  • Consistently made the runners ubuntu-20.04. Right now, deployment workflows use 22.04 and integration uses 20.04. This is not very relevant for NodeJS, but can be relevant for libindy/ssl/pool setup
  • set-output as a statement in Github Actions is deprecated, and is currently in the sunset/transition period. It will be fully removed on 31st May 2023. I've updated these statements to use the new recommended technique instead, otherwise, all CI/CD would start failing in May 2023.
  • Added --frozen-lockfile to all yarn install statements. Otherwise, there's a non-zero chance the target dependency resolution is different than what's in yarn.lock and could cause divergence in the lint/build stage vs what's happening in release.
  • Bumped Actions versions, where needed

CodeQL

CodeQL requires a build to be executed to analyse security and quality issues correctly. I've added a build statement to the CodeQL workflow, and added query packs explicitly (the default is security-extended). I've also updated the workflow versions for security.

Dependabot

Adds a Dependabot workflow for security and version bumps. I've set the strategy to focus only on direct dependencies, rather than direct + devDependencies, although this can be customised.

New cache cleanup workflow

Github repositories have a maximum storage of 10 GB allocated to them with 7 days worth of caching, beyond which entries get ejected. However, if within those 7 days the cache hits 10 GB, there's a chance that CI/CD workflows could fail. I've added a workflow to evict entries from the cache every 3 days, which will make the whole repository less likely to hit the cache limit and have workflows fail. (This can also be an issue for forks.)

Lint PR, Repo Linter

Bumped external Actions to latest versions

Dockerfile

I'm not sure if this is used still (or perhaps for local development only), but I recommend some of the following optimisations since the built image size is approx 4.5 GB (which is pretty big):

  • Combine RUN statements. Each RUN, COPY, ADD etc adds layers, and the Docker caching engine behaves better (i.e., will likely find more cache hits for combined statements)
  • Right now, the Dockerfile isn't a true multi-stage build since the base target is just copied over as-is to the final target. This doesn't really optimise the final image size. For example, one method would be keeping base mostly as-is, and then using the node:16-buster or node:16-buster-slim as the base for final stage. Built artefacts from base can be copied over using COPY --from=base... syntax. This way, the larger base image with all its dependencies and fat Ubuntu image can be ditched to make a slimmer final image. However, I'm not exactly sure which bits need to be copied over so I couldn't execute this entirely. Can the yarn build be done independently, or does it require the pre-requisites?
  • For the final stage image, if possible, I'd recommend using node:16-alpine instead. While there's more packages that need to be installed (e.g., bash is not installed of the box, it comes withsh only) the image sizes are dramatically lower (approx. 50-100 MB vs 4+ GB on other repos where I've implemented this).

@TimoGlastra
Copy link
Contributor

Thanks for the PR @ankurdotb! This is great and a much needed update

Keep local copy of setup-node Action or ditch it? (Explained below)

Ditch it!

actions/setup-libssl seems to be adding a package designed for Ubuntu 18.04. Ideally, this should be changed to the package for Ubuntu 20.04 or Ubuntu 22.04 (depending on runner decision above).

I wasn't aware we were using this action. If we can use ubuntu 22.04 that is best I think?

Althought there's a pr open from ry that adds a bigger runner for ubuntu-latest which speeds up the PRs and may also solve some of the ci issues

Use semantic-release to auto-calculate version numbers and whether to release? (See releaserc.json definition in @cheqd/sdk for an example). This uses a custom/default Convention Commit convention to auto-trigger releases.

We use semantic release (well we use lerna, which uses conventional commits), but we want to release an alpha version on every PR to main, which calculates the correct next version. We also don't store the alpha version in the package json as that requires updating the package json for every PR. So that's the story of how we got this CI, if you have a way to keep the same behavior, but simplify I'm all in!

Should job runners be consistently set to ubuntu-20.04 or ubuntu-22.04? (ubuntu-latest now points to ubuntu-22.04. There was a transition period when it could have pointed to either near end of 2022.)

See my comment above. 22.04 is fine, but we may want to use the large runner that a PR is open for now.

@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2023

Codecov Report

Merging #1428 (ea3f887) into main (b35fec4) will increase coverage by 3.43%.
The diff coverage is n/a.

❗ Current head ea3f887 differs from pull request most recent head 095abb8. Consider uploading reports for the commit 095abb8 to get more accurate results

@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
+ Coverage   82.42%   85.86%   +3.43%     
==========================================
  Files         870      870              
  Lines       20489    20489              
  Branches     3449     3449              
==========================================
+ Hits        16888    17592     +704     
+ Misses       3365     2725     -640     
+ Partials      236      172      -64     

see 18 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@genaris
Copy link
Contributor

genaris commented Apr 12, 2023

actions/setup-libssl seems to be adding a package designed for Ubuntu 18.04. Ideally, this should be changed to the package for Ubuntu 20.04 or Ubuntu 22.04 (depending on runner decision above).

I wasn't aware we were using this action. If we can use ubuntu 22.04 that is best I think?

The action was added because we need Ubuntu 22.04 for anoncreds-rs (see hyperledger/anoncreds-rs#105) but indy-sdk doesn't support the shipped libssl version, so we needed to downgrade to libssl 1.1.1. I think it can be changed to libssl1.1_1.1.1f-1ubuntu2_amd64.deb although IMHO the best solution in the short term would be to fix the issue in anoncreds-rs and be able to use ubuntu 20.04 again.

I think in the mid-term ideally we should not depend anymore on indy-sdk (at least in this repo) so we would be able to use ubuntu 22.04 or even node:18 or so.

@ankurdotb
Copy link
Contributor Author

@TimoGlastra (and also @ryjones)

Ditch it!

Done! I've deleted the local copy of actions/setup-node. Actual logs from CI runs should confirm whether it's working okay (we should find cache hits).

Althought there's a pr open from ry that adds a bigger runner for ubuntu-latest which speeds up the PRs and may also solve some of the ci issues

Yes, I saw #1429. @ryjones, is ubuntu-latest-m a custom large runner name, or a new unreleased feature you have access to? Adding a -m at the end to get a larger runner is not syntax I'm familiar with, since the default larger runner labels are different.

IMO if you running into issues with the runners, these two points below without requiring larger runners:

  1. The 10 GB cache limit could have been causing ejections, and if too full it might even stop some workflows. Cache cleanup every 3 days will alleviate some of this. Anyone who's admin on this repo will be able to see the size of the cache, if it gets up to 10 GB earlier than 3 days, we can dial it down to 1-2 days instead.
  2. The other thing that's really helped on some of our repos with very large build sizes/times is limiting the heap memory allocated using NODE_OPTIONS: --max_old_space_size=4096. Standard Github runners have 2 CPU, 7 GB RAM which is honestly not that bad. (I'll bump it up from 4 GB to 6 GB.) Previously, this was only set in one of the pipelines and not the other one.

I wasn't aware we were using this action. If we can use ubuntu 22.04 that is best I think?
...so we needed to downgrade to libssl 1.1.1. I think it can be changed to libssl1.1_1.1.1f-1ubuntu2_amd64.deb although IMHO the best solution in the short term would be to fix the issue in anoncreds-rs and be able to use ubuntu 20.04 again.

libssl is installed when uses: ./.github/actions/setup-libssl is called in the workflow. The action then downloads the Debian package "libssl1.1_1.1.1-1ubuntu2.1~18.04.21_amd64.deb". This is where it's calling the libssl library that was targetted for Ubuntu 18.04.

I would recommend downgrading the runners to Ubuntu 20.04, thanks for the context @genaris. If I look up libssl on APT package search:

  1. Ubuntu 22.04 "Jammy" is on libssl3 with no backports available for libssl1.1
  2. Ubuntu 20.04 "Focal" does have libssl1.1 available, as long as that specific version works. I would recommend this, since the currently-used libssl is old version for 18.04 with vulnerabilities and this is a critical chain in the communication.

If we do need larger runners, @ryjones might need to redefine the "large" runners to be Ubuntu 20.04 instead.

We use semantic release (well we use lerna, which uses conventional commits), but we want to release an alpha version on every PR to main, which calculates the correct next version. We also don't store the alpha version in the package json as that requires updating the package json for every PR. So that's the story of how we got this CI, if you have a way to keep the same behavior, but simplify I'm all in!

Do you mean you're using the semantic release convention? Since I don't see this repo using the semantic-release NPM package. Our setup is:

  1. package.json does get updated, and is inserted by Semantic Release with the text [skip ci]. This suppresses triggering another release when the package is published.
  2. If you want to set the release on Github to pre-release, test it for a sanity check, then toggle it to latest - you can set the default to do pre-release only and then manually toggle the release flag.

I think maybe we can split this semantic release stuff out to a different PR, since it might be helpful to walk you or whoever else does releases here with the semantic-release NPM package configuration options.

Lastly, I edited my original message to add some detail around some changes I made to the Dockerfile. Right now, I can see that the Dockerfile images are pretty large (about 4+ GB) and I have some suggestions there on how to do fully utilise Docker's multi-stage build capability. Could you please take a look at that section @TimoGlastra? I need some support though to understand what needs to be copied from build stage (dependencies, artefacts) to a final, slimmer image. I did try it, but didn't commit it since I kept running into build issues with node-gyp which seems similar to this best practice mitigation in Node.js Docker best practices guide (even though it's for their alpine images).

ankurdotb and others added 23 commits April 13, 2023 10:04
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
- Bump actions/checkout
- Switch to `--immutable` with yarn (this is the new command)
- Fix deprecated set-output command

Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
- Use a consistent runner
- Fix set-output issues
- Switch to setup-node

Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
build: test large builders

Signed-off-by: Ry Jones <ry@linux.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Signed-off-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
@ryjones
Copy link
Contributor

ryjones commented Apr 14, 2023

@ankurdotb @TimoGlastra AFJ has access to

  • ubuntu-latest-m 4-cores · 16 GB RAM · 150 GB SSD - Image: Ubuntu Latest (22.04)
  • ubuntu-2004-paid 4-cores · 16 GB RAM · 150 GB SSD - Image: Ubuntu 20.04
  • windows-latest-l 8-cores · 32 GB RAM · 300 GB SSD - Image: Windows Latest (2022)

These are paid as a test.

@ryjones
Copy link
Contributor

ryjones commented Apr 14, 2023

@ankurdotb : here is the output prettier:

index d9faf80..eb9c53f 100644
--- a/.github/dependabot.yml
+++ b/.github/dependabot.yml
@@ -4,30 +4,29 @@
 
 version: 2
 updates:
-
   # Maintain dependencies for NPM
-  - package-ecosystem: "npm"
-    directory: "/"
+  - package-ecosystem: 'npm'
+    directory: '/'
     schedule:
-      interval: "monthly"
+      interval: 'monthly'
     allow:
-    # Focus on main dependencies, not devDependencies
-      - dependency-type: "production"
+      # Focus on main dependencies, not devDependencies
+      - dependency-type: 'production'
 
   # Maintain dependencies for GitHub Actions
-  - package-ecosystem: "github-actions"
-    directory: "/"
+  - package-ecosystem: 'github-actions'
+    directory: '/'
     schedule:
-      interval: "monthly"
+      interval: 'monthly'
 
   # Maintain dependencies for Docker
-  - package-ecosystem: "docker"
-    directory: "/"
+  - package-ecosystem: 'docker'
+    directory: '/'
     schedule:
-      interval: "monthly"
+      interval: 'monthly'
 
   # Maintain dependencies for Cargo
-  - package-ecosystem: "cargo"
-    directory: "/"
+  - package-ecosystem: 'cargo'
+    directory: '/'
     schedule:
-      interval: "monthly"
+      interval: 'monthly'
diff --git a/.github/workflows/cleanup-cache.yml b/.github/workflows/cleanup-cache.yml
index 31bb3f8..4b4ecd5 100644
--- a/.github/workflows/cleanup-cache.yml
+++ b/.github/workflows/cleanup-cache.yml
@@ -1,17 +1,16 @@
 # Repositories have 10 GB of cache storage per repository
 # Documentation: https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#usage-limits-and-eviction-policy
-name: "Cleanup - Cache"
+name: 'Cleanup - Cache'
 on:
   schedule:
-    - cron: "0 0 * * 0/3"
+    - cron: '0 0 * * 0/3'
   workflow_dispatch:
 
 jobs:
-
   delete-caches:
-    name: "Delete Actions caches"
+    name: 'Delete Actions caches'
     runs-on: ubuntu-latest
-    
+
     steps:
-      - name: "Wipe Github Actions cache"
+      - name: 'Wipe Github Actions cache'
         uses: easimon/wipe-cache@v2
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 737b35c..cdebeb2 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -14,7 +14,6 @@ concurrency:
 env:
   NODE_OPTIONS: --max_old_space_size=4096
 
-
 jobs:
   analyze:
     name: Analyze

Copy link
Contributor

@ryjones ryjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the runner names changed again (for the last time).

jobs:
release-canary:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest-m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runs-on: aries-ubuntu-latest

@@ -56,22 +61,23 @@ jobs:
git push origin v${{ steps.get-version.outputs.version }} --no-verify

release-stable:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest-m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runs-on: aries-ubuntu-latest

cancel-in-progress: true

jobs:
# PRs created by github actions won't trigger CI. Before we can merge a PR we need to run the tests and
# validation scripts. To still be able to run the CI we can manually trigger it by adding the 'ci-test'
# label to the pull request
ci-trigger:
runs-on: ubuntu-22.04
runs-on: ubuntu-latest-m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runs-on: aries-ubuntu-latest


validate:
runs-on: ubuntu-22.04
runs-on: ubuntu-latest-m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runs-on: aries-ubuntu-latest

@@ -79,7 +81,7 @@ jobs:
run: yarn build

integration-test:
runs-on: ubuntu-22.04
runs-on: ubuntu-latest-m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runs-on: aries-ubuntu-latest


- uses: codecov/codecov-action@v1
if: always()

version-stable:
runs-on: ubuntu-22.04
runs-on: ubuntu-latest-m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runs-on: aries-ubuntu-latest

@TimoGlastra
Copy link
Contributor

Discussed on AFJ WG call.

@ankurdotb do you have time to resolve the final comments from @ryjones? If not @genaris can take a look at the final pieces

@TimoGlastra
Copy link
Contributor

Superseded by #1500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants