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

chore: improve workflow #455

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fresh-turkeys-grow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperlane-xyz/registry': patch
---

Improve CI workflow by separating concerns
Xaroz marked this conversation as resolved.
Show resolved Hide resolved
46 changes: 46 additions & 0 deletions .github/workflows/validate.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
name: validate-files

on:
push:
branches: ["main"]
pull_request:
types: [opened, synchronize, reopened, ready_for_review]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why narrow this job down to these types?

Copy link
Contributor Author

@Xaroz Xaroz Dec 26, 2024

Choose a reason for hiding this comment

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

The idea was to have the CI auto-run for every PR to the repo and this would prevent the CI to run on PRs that didn't meet these criteria. I know we discarded the idea of having it auto-run on every third-party PR, but I think this is still good, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with having it run on every PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial plan for this was that if we would change the permissions so that we wouldn't need to approve a workflow for it to run, so by doing these we would make sure only these types of PR would auto-run the workflow

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to approve the workflows for security reasons, I don't think narrowing down the PR type will help. I recommend cutting this but it's up to you.

# Allows you to run this workflow manually
workflow_dispatch:

jobs:
validate-files:
runs-on: ubuntu-latest
permissions:
contents: read
steps:
- uses: actions/checkout@v4
- uses: actions/cache@v4
with:
path: |
**/node_modules
.yarn/cache
key: ${{ runner.os }}-yarn-cache-${{ hashFiles('./yarn.lock') }}

- name: yarn-install
run: |
yarn install
CHANGES=$(git status -s)
if [[ ! -z $CHANGES ]]; then
echo "Changes found: $CHANGES"
git diff
exit 1
fi

- name: setup-node
uses: actions/setup-node@v3
with:
node-version: 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, what happens if you remove this? I think the ubuntu image already has node. That's why the CI job doesn't include this step and why the yarn install step before it works.

Copy link
Contributor Author

@Xaroz Xaroz Dec 26, 2024

Choose a reason for hiding this comment

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

So doing a bit more digging, yes it does work but apparently the reason people still include this is that they don't want to rely on the default that comes installed with ubuntu-latest and have a bit more of a controlled environment. Other than that it also manages the Node.js installation in a way that is more optimized for Github Actions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand but we should be consistent. Either use it in all the workflows or none. And if you're going to add it, use actions/setup-node@v4 which is the latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed here, I will look into adding it to all the other workflows on another PR, we can discuss this further


- name: validate-file-path
run: |
node ./scripts/validate-file-path.js

- name: validate-svg
run: |
node ./scripts/validate-svg.js
34 changes: 34 additions & 0 deletions scripts/common.js
jmrossy marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import fs from 'fs';
import path from 'path';

function findFiles(directory, fileTypes = [], isRecursive = true) {
const files = fs.readdirSync(directory);

return files.flatMap((file) => {
const fullPath = path.join(directory, file);
const stats = fs.statSync(fullPath);

if (stats.isDirectory() && isRecursive) {
return findFiles(fullPath, fileTypes); // Recurse into subdirectories
} else if (fileTypes.includes(path.extname(fullPath))) {
return fullPath;
}

return [];
});
}

// Get all fille paths for given directories and given file types
export function getFilePaths(directories = [], fileTypes = [], isRecursive = true) {
return directories
.filter((directory) => {
if (fs.existsSync(directory)) {
console.log(`Checking directory: ${directory}`);
return true;
} else {
console.log(`Directory does not exist: ${directory}`);
return false;
}
})
.flatMap((directory) => findFiles(directory, fileTypes, isRecursive));
}
92 changes: 3 additions & 89 deletions scripts/optimize-svg.js
Original file line number Diff line number Diff line change
@@ -1,95 +1,10 @@
import fs from 'fs';
import path from 'path';
import { optimize } from 'svgo';
import { getFilePaths } from './common.js';

const directories = ['./chains', './deployments'];
const MAX_FILE_SIZE = 100 * 1024; // 100KBs
const RASTER_IMAGE_REGEX = /<image[^>]*>/i;

const invalidNameSVGs = [];
const invalidSizeSVGs = [];
const rasterImgSVGs = [];

function isValidSvg(filePath) {
const fileName = path.basename(filePath);
const stats = fs.statSync(filePath);
const fileSize = (stats.size / 1024).toFixed(2);

if (!fileName.endsWith('logo.svg')) {
invalidNameSVGs.push(filePath);
}

if (stats.size > MAX_FILE_SIZE) {
invalidSizeSVGs.push({ filePath, fileSize: `${fileSize}KBs` });
}

const fileContent = fs.readFileSync(filePath, 'utf8');
if (RASTER_IMAGE_REGEX.test(fileContent)) {
rasterImgSVGs.push(filePath);
}
}

// Finds all svgs, validates and return all paths found that has svgs
function findAndValidateSVGs(directory) {
const files = fs.readdirSync(directory);

return files.flatMap((file) => {
const fullPath = path.join(directory, file);
const stats = fs.statSync(fullPath);

if (stats.isDirectory()) {
return findAndValidateSVGs(fullPath); // Recurse into subdirectories
} else if (path.extname(fullPath) === '.svg') {
isValidSvg(fullPath);
return fullPath;
}

return [];
});
}

// Get all svg paths that are validated
function getSVGPaths() {
return directories
.filter((directory) => {
if (fs.existsSync(directory)) {
console.log(`Checking directory: ${directory}`);
return true;
} else {
console.log(`Directory does not exist: ${directory}`);
return false;
}
})
.flatMap((directory) => findAndValidateSVGs(directory));
}

function validateErrors() {
const errorCount = invalidNameSVGs.length + invalidSizeSVGs.length + rasterImgSVGs.length;
if (errorCount === 0) return;

console.error(`Number of errors found: ${errorCount}`);

if (invalidNameSVGs.length > 0) {
console.error(
"Error: Files do not end with 'logo.svg' in the following paths:",
invalidNameSVGs,
);
}

if (invalidSizeSVGs.length > 0) {
console.error('Error: Files size exceed 100KBs in:', invalidSizeSVGs);
}

if (rasterImgSVGs.length > 0) {
console.error(
'Error: Files contain an <image> tag, likely embedding a raster image in the following paths:',
rasterImgSVGs,
);
}

process.exit(1);
}
// Optimize svg in given path
// Optimize svg in given paths
function optimizeSVGs(svgPaths) {
svgPaths.forEach((filePath) => {
try {
Expand Down Expand Up @@ -122,8 +37,7 @@ function optimizeSVGs(svgPaths) {
}

function main() {
const svgPaths = getSVGPaths();
validateErrors();
const svgPaths = getFilePaths(directories, ['.svg']);
optimizeSVGs(svgPaths);
}

Expand Down
24 changes: 24 additions & 0 deletions scripts/validate-file-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { getFilePaths } from './common.js';

const directories = [
{ paths: ['./src'], recursive: true },
{ paths: ['./'], recursive: false },
];

const fileExtensions = ['.svg', '.yaml'];

function main() {
const invalidFilesPaths = directories.flatMap((directory) =>
getFilePaths(directory.paths, fileExtensions, directory.recursive),
);

if (invalidFilesPaths.length === 0) return;

console.error(
'Error: invalid file paths found, make sure they are in the proper directories (chains or deployments):',
invalidFilesPaths,
);
process.exit(1);
}

main();
69 changes: 69 additions & 0 deletions scripts/validate-svg.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import fs from 'fs';
import path from 'path';
import { getFilePaths } from './common.js';

const directories = ['./chains', './deployments'];
const MAX_FILE_SIZE = 100 * 1024; // 100KBs
const RASTER_IMAGE_REGEX = /<image[^>]*>/i;

const invalidNameSVGs = [];
const invalidSizeSVGs = [];
const rasterImgSVGs = [];

function isValidSvg(filePath) {
const fileName = path.basename(filePath);
const stats = fs.statSync(filePath);
const fileSize = (stats.size / 1024).toFixed(2);

if (!fileName.endsWith('logo.svg')) {
invalidNameSVGs.push(filePath);
}

if (stats.size > MAX_FILE_SIZE) {
invalidSizeSVGs.push({ filePath, fileSize: `${fileSize}KBs` });
}

const fileContent = fs.readFileSync(filePath, 'utf8');
if (RASTER_IMAGE_REGEX.test(fileContent)) {
rasterImgSVGs.push(filePath);
}
}

function validateErrors() {
const errorCount = invalidNameSVGs.length + invalidSizeSVGs.length + rasterImgSVGs.length;
if (errorCount === 0) return;

console.error(`Number of errors found: ${errorCount}`);

if (invalidNameSVGs.length > 0) {
console.error(
"Error: Files do not end with 'logo.svg' in the following paths:",
invalidNameSVGs,
);
}

if (invalidSizeSVGs.length > 0) {
console.error('Error: Files size exceed 100KBs in:', invalidSizeSVGs);
}

if (rasterImgSVGs.length > 0) {
console.error(
'Error: Files contain an <image> tag, likely embedding a raster image in the following paths:',
rasterImgSVGs,
);
}

process.exit(1);
}

function validateSvgs(paths = []) {
paths.forEach((path) => isValidSvg(path));
validateErrors();
}

function main() {
const svgPaths = getFilePaths(directories, ['.svg']);
validateSvgs(svgPaths);
}

main();
Loading