Skip to content

add make-generate to pre-commit #491

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 2, 2024
Merged

add make-generate to pre-commit #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions .github/workflows/pre-commit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,18 @@ jobs:
- name: Install pre-commit
run: pip install pre-commit

- name: Install generate
run: |
sudo apt update
sudo apt install curl -y
curl -fsSL https://deb.nodesource.com/setup_16.x | sudo -E bash -
sudo apt install nodejs -y
git clone https://github.com/bitnami/readme-generator-for-helm
cd ./readme-generator-for-helm
npm install
npm install -g pkg
pkg . -o /usr/local/bin/readme-generator
Comment on lines +20 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security and stability improvements needed for generate tool installation

Several concerns with the current implementation:

  1. Using Node.js 16 which is approaching EOL
  2. No version pinning for the external repository
  3. Direct sudo usage in GitHub Actions
  4. No verification of downloaded content

Consider these improvements:

       - name: Install generate
         run: |
-          sudo apt update
-          sudo apt install curl -y
-          curl -fsSL https://deb.nodesource.com/setup_16.x | sudo -E bash -
-          sudo apt install nodejs -y
-          git clone https://github.com/bitnami/readme-generator-for-helm
+          sudo apt-get update
+          sudo apt-get install -y curl
+          curl -fsSL https://deb.nodesource.com/setup_20.x | sudo -E bash -
+          sudo apt-get install -y nodejs
+          # Clone specific version and verify
+          git clone --depth 1 --branch v1.0.0 https://github.com/bitnami/readme-generator-for-helm
           cd ./readme-generator-for-helm
+          # Verify repository content
+          git verify-commit HEAD || echo "Warning: Commit signature verification failed"
           npm install
           npm install -g pkg
           pkg . -o /usr/local/bin/readme-generator

Also consider:

  1. Using GitHub's official Node.js setup action instead of manual installation
  2. Caching npm dependencies
  3. Adding SHA256 verification for downloaded content

Committable suggestion skipped: line range outside the PR's diff.


- name: Run pre-commit hooks
run: |
git fetch origin main || git fetch origin master
Expand Down
17 changes: 15 additions & 2 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
repos:
- repo: local
hooks:
- repo: local
hooks:
- id: gen-versions-map
name: Generate versions map and check for changes
entry: sh -c 'make -C packages/apps check-version-map && make -C packages/extra check-version-map'
language: system
types: [file]
pass_filenames: false
description: Run the script and fail if it generates changes
- id: run-make-generate
name: Run 'make generate' in all app directories
entry: |
/bin/bash -c '
for dir in ./packages/apps/*/; do
if [ -d "$dir" ]; then
echo "Running make generate in $dir"
(cd "$dir" && make generate)
fi
done
'
language: script
files: ^.*$
Comment on lines +11 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and documentation for make-generate hook

The new hook implementation has several areas for improvement:

  1. No error handling for make generate failures
  2. Runs on all files (^.*$) which might impact performance
  3. Missing documentation about what 'make generate' does

Consider this improved implementation:

     - id: run-make-generate
       name: Run 'make generate' in all app directories
       entry: |
         /bin/bash -c '
+          set -e  # Exit on any error
+          if ! command -v make >/dev/null 2>&1; then
+            echo "Error: make is not installed"
+            exit 1
+          fi
           for dir in ./packages/apps/*/; do
             if [ -d "$dir" ]; then
               echo "Running make generate in $dir"
-              (cd "$dir" && make generate)
+              (cd "$dir" && make generate) || {
+                echo "Error: make generate failed in $dir"
+                exit 1
+              }
             fi
           done
         '
       language: script
-      files: ^.*$
+      files: ^packages/apps/.*$

Also, please add documentation about what 'make generate' does and its purpose.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- id: run-make-generate
name: Run 'make generate' in all app directories
entry: |
/bin/bash -c '
for dir in ./packages/apps/*/; do
if [ -d "$dir" ]; then
echo "Running make generate in $dir"
(cd "$dir" && make generate)
fi
done
'
language: script
files: ^.*$
- id: run-make-generate
name: Run 'make generate' in all app directories
entry: |
/bin/bash -c '
set -e # Exit on any error
if ! command -v make >/dev/null 2>&1; then
echo "Error: make is not installed"
exit 1
fi
for dir in ./packages/apps/*/; do
if [ -d "$dir" ]; then
echo "Running make generate in $dir"
(cd "$dir" && make generate) || {
echo "Error: make generate failed in $dir"
exit 1
}
fi
done
'
language: script
files: ^packages/apps/.*$

7 changes: 1 addition & 6 deletions packages/apps/nats/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,4 @@
| `external` | Enable external access from outside the cluster | `false` |
| `replicas` | Persistent Volume size for NATS | `2` |
| `storageClass` | StorageClass used to store the data | `""` |

### Configuration parameters

| Name | Description | Value |
| ----------- | ----------------------- | ----- |
| `users` | Users configuration | `{}` |
| `users` | Users configuration | `{}` |
44 changes: 15 additions & 29 deletions packages/apps/nats/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,34 +2,20 @@
"title": "Chart Values",
"type": "object",
"properties": {
"external": {
"type": "boolean",
"description": "Enable external access from outside the cluster",
"default": false
},
"replicas": {
"type": "number",
"description": "Persistent Volume size for NATS",
"default": 2
},
"storageClass": {
"type": "string",
"description": "StorageClass used to store the data",
"default": ""
},
"users": {
"type": "object",
"description": "Users configuration",
"additionalProperties": {
"type": "object",
"properties": {
"password": {
"type": "string",
"description": "Password for the user"
}
}
"external": {
"type": "boolean",
"description": "Enable external access from outside the cluster",
"default": false
},
"default": {}
}
"replicas": {
"type": "number",
"description": "Persistent Volume size for NATS",
"default": 2
},
"storageClass": {
"type": "string",
"description": "StorageClass used to store the data",
"default": ""
}
Comment on lines +5 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Removal of users property from schema conflicts with active usage

The users property is actively used in both values.yaml (as a default empty configuration) and templates/nats.yaml (for generating credentials and NATS configuration). Removing it from the schema while keeping the functionality in the templates would lead to:

  • Undocumented but required configuration option
  • Potential breaking changes for existing deployments using user authentication
  • Schema validation errors for valid configurations

Locations needing attention:

  • values.schema.json: Keep the users property in the schema
  • If removal is intended, corresponding changes are needed in:
    • values.yaml: Remove the users configuration
    • templates/nats.yaml: Remove the user authentication logic
🔗 Analysis chain

Schema structure looks good but verify impact of users removal

The schema properties are well-structured with clear descriptions and appropriate types. However, the removal of the users property might impact existing configurations.

Let's verify the impact of removing the users configuration:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any references to NATS users configuration
rg -l "users:" ./packages/apps/nats/

Length of output: 110

Script:

#!/bin/bash
# Check the content of values.yaml and templates/nats.yaml to understand the usage of users configuration
echo "=== values.yaml ==="
cat ./packages/apps/nats/values.yaml

echo -e "\n=== templates/nats.yaml ==="
cat ./packages/apps/nats/templates/nats.yaml

Length of output: 2449

}
}
}
2 changes: 1 addition & 1 deletion packages/apps/postgres/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,4 @@
}
}
}
}
}