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.

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

add e2e for apps #426

Closed
wants to merge 3 commits into from
Closed

add e2e for apps #426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e274032
add e2e for apps
klinch0 Oct 15, 2024
03206eb
Refactor e2e tests for applications
kvaps Oct 18, 2024
fe80232
Merge pull request #1 from aenix-io/add-tests-refactor
klinch0 Oct 18, 2024
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
15 changes: 12 additions & 3 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,19 @@ repos:
- id: mixed-line-ending
args: [--fix=lf]
- id: check-yaml
exclude: packages/apps/postgres/templates/init-script.yaml
exclude: .*/init-script\.yaml$
args: [--unsafe]
- repo: https://github.com/igorshubovych/markdownlint-cli
rev: v0.41.0
hooks:
- id: markdownlint
args: [--fix, --disable, MD013, MD041, --]
- id: markdownlint
args: [--fix, --disable, MD013, MD041, --]
- repo: local
hooks:
- id: gen-versions-map
name: Generate versions map and check for changes
entry: bash -c 'cd packages/apps && make check-version-map'
language: system
types: [file]
pass_filenames: false
description: Run the script and fail if it generates changes
16 changes: 16 additions & 0 deletions hack/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.PHONY: test clean help

SCRIPT=./e2e.applications.sh
PRECHECKS=./pre-checks.sh

help:
@echo "Usage: make {test|clean}"
@echo " test - Run the end-to-end tests."
@echo " clean - Clean up resources."

test:
@bash $(PRECHECKS) test
@bash $(SCRIPT) test

clean:
@bash $(SCRIPT) clean
47 changes: 47 additions & 0 deletions hack/e2e.applications.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
for file in ./modules/*.sh; do
source "$file"
done
Comment on lines +1 to +3
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

Improve script initialization and error handling

Consider the following improvements:

  1. Add a shebang line at the beginning of the script.
  2. Implement error handling for missing modules.
  3. Set shell options for better error detection.

Here's a suggested improvement:

+#!/bin/bash
+set -euo pipefail
+
+# Source all modules required for this script
 for file in ./modules/*.sh; do
+    if [[ ! -f "$file" ]]; then
+        echo "Error: Required module $file not found" >&2
+        exit 1
+    fi
     source "$file"
 done

This change addresses the static analysis warning about the missing shebang and improves overall script robustness.

📝 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
for file in ./modules/*.sh; do
source "$file"
done
#!/bin/bash
set -euo pipefail
# Source all modules required for this script
for file in ./modules/*.sh; do
if [[ ! -f "$file" ]]; then
echo "Error: Required module $file not found" >&2
exit 1
fi
source "$file"
done
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

[warning] 2-2: ShellCheck can't follow non-constant source. Use a directive to specify location.

(SC1090)


ROOT_NS="tenant-root"
TEST_TENANT="tenant-e2e"

function test() {
install_tenant $TEST_TENANT $ROOT_NS
check_helmrelease_status $TEST_TENANT $ROOT_NS

install_all_apps "../packages/apps" "$TEST_TENANT" cozystack-apps cozy-public

if true; then
Copy link
Member

Choose a reason for hiding this comment

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

This will always be true, isn't that?

echo -e "${GREEN}All tests passed!${RESET}"
return 0
else
echo -e "${RED}Some tests failed!${RESET}"
return 1
fi
}
Comment on lines +8 to +21
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

Improve test function logic and error handling

The current implementation of the test function has several issues:

  1. It always reports success regardless of the actual test results.
  2. The error case (lines 17-20) is unreachable due to the if true condition.
  3. There are no actual test assertions or checks for the installed applications.

Consider refactoring the function to include proper error handling and test assertions. Here's a suggested improvement:

 function test() {
-    install_tenant $TEST_TENANT $ROOT_NS
-    check_helmrelease_status $TEST_TENANT $ROOT_NS
+    if ! install_tenant $TEST_TENANT $ROOT_NS; then
+        echo -e "${RED}Failed to install tenant${RESET}"
+        return 1
+    fi
+
+    if ! check_helmrelease_status $TEST_TENANT $ROOT_NS; then
+        echo -e "${RED}Helm release status check failed${RESET}"
+        return 1
+    fi

-    install_all_apps "../packages/apps" "$TEST_TENANT" cozystack-apps cozy-public
+    if ! install_all_apps "../packages/apps" "$TEST_TENANT" cozystack-apps cozy-public; then
+        echo -e "${RED}Failed to install all apps${RESET}"
+        return 1
+    fi

-    if true; then
-        echo -e "${GREEN}All tests passed!${RESET}"
-        return 0
-    else
-        echo -e "${RED}Some tests failed!${RESET}"
-        return 1
-    fi
+    # Add actual test assertions here
+    # For example:
+    # if ! check_app_status app1 $TEST_TENANT; then
+    #     echo -e "${RED}App1 status check failed${RESET}"
+    #     return 1
+    # fi
+
+    echo -e "${GREEN}All tests passed!${RESET}"
+    return 0
 }

This refactoring adds proper error handling and allows for the addition of specific test assertions for the installed applications.

Committable suggestion was skipped due to low confidence.


function clean() {
kubectl delete helmrelease.helm.toolkit.fluxcd.io $TEST_TENANT -n $ROOT_NS
if true; then
Copy link
Member

Choose a reason for hiding this comment

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

This will always be true, isn't that?

echo -e "${GREEN}Cleanup successful!${RESET}"
return 0
else
echo -e "${RED}Cleanup failed!${RESET}"
return 1
fi
}
Comment on lines +23 to +32
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

Improve clean function logic and error handling

The clean function has similar issues to the test function:

  1. It always reports success regardless of the actual cleanup result.
  2. The error case (lines 29-31) is unreachable due to the if true condition.
  3. There's no error handling for the kubectl delete command.

Consider refactoring the function to include proper error handling. Here's a suggested improvement:

 function clean() {
-    kubectl delete helmrelease.helm.toolkit.fluxcd.io $TEST_TENANT -n $ROOT_NS
-    if true; then
+    if kubectl delete helmrelease.helm.toolkit.fluxcd.io $TEST_TENANT -n $ROOT_NS; then
         echo -e "${GREEN}Cleanup successful!${RESET}"
         return 0
     else
         echo -e "${RED}Cleanup failed!${RESET}"
         return 1
     fi
 }

This refactoring adds proper error handling for the cleanup operation.

📝 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
function clean() {
kubectl delete helmrelease.helm.toolkit.fluxcd.io $TEST_TENANT -n $ROOT_NS
if true; then
echo -e "${GREEN}Cleanup successful!${RESET}"
return 0
else
echo -e "${RED}Cleanup failed!${RESET}"
return 1
fi
}
function clean() {
if kubectl delete helmrelease.helm.toolkit.fluxcd.io $TEST_TENANT -n $ROOT_NS; then
echo -e "${GREEN}Cleanup successful!${RESET}"
return 0
else
echo -e "${RED}Cleanup failed!${RESET}"
return 1
fi
}


case "$1" in
test)
echo -e "${YELLOW}Running tests...${RESET}"
test
;;
clean)
echo -e "${YELLOW}Cleaning up...${RESET}"
clean
;;
*)
echo -e "${RED}Usage: $0 {test|clean}${RESET}"
exit 1
;;
esac
Comment on lines +34 to +47
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

Improve main script logic to handle function return values

The main script logic correctly uses a case statement to handle command-line arguments. However, it doesn't capture or handle the return values from the test and clean functions.

Consider modifying the script to capture and handle the return values:

 case "$1" in
     test)
         echo -e "${YELLOW}Running tests...${RESET}"
-        test
+        if test; then
+            echo -e "${GREEN}Tests completed successfully${RESET}"
+            exit 0
+        else
+            echo -e "${RED}Tests failed${RESET}"
+            exit 1
+        fi
         ;;
     clean)
         echo -e "${YELLOW}Cleaning up...${RESET}"
-        clean
+        if clean; then
+            echo -e "${GREEN}Cleanup completed successfully${RESET}"
+            exit 0
+        else
+            echo -e "${RED}Cleanup failed${RESET}"
+            exit 1
+        fi
         ;;
     *)
         echo -e "${RED}Usage: $0 {test|clean}${RESET}"
         exit 1
         ;;
 esac

This modification ensures that the script's exit status reflects the success or failure of the test and clean operations, which is crucial for integration with CI/CD pipelines or other automated processes.

📝 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
case "$1" in
test)
echo -e "${YELLOW}Running tests...${RESET}"
test
;;
clean)
echo -e "${YELLOW}Cleaning up...${RESET}"
clean
;;
*)
echo -e "${RED}Usage: $0 {test|clean}${RESET}"
exit 1
;;
esac
case "$1" in
test)
echo -e "${YELLOW}Running tests...${RESET}"
if test; then
echo -e "${GREEN}Tests completed successfully${RESET}"
exit 0
else
echo -e "${RED}Tests failed${RESET}"
exit 1
fi
;;
clean)
echo -e "${YELLOW}Cleaning up...${RESET}"
if clean; then
echo -e "${GREEN}Cleanup completed successfully${RESET}"
exit 0
else
echo -e "${RED}Cleanup failed${RESET}"
exit 1
fi
;;
*)
echo -e "${RED}Usage: $0 {test|clean}${RESET}"
exit 1
;;
esac

Comment on lines +1 to +47
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 overall script structure and follow bash best practices

To improve the script's readability, maintainability, and robustness, consider the following suggestions:

  1. Add comments explaining the purpose of the script and its functions.
  2. Follow bash best practices, such as quoting variables.
  3. Add proper initialization and error handling for sourced modules.

Here's an example of how you could improve the script's structure:

#!/bin/bash
set -euo pipefail

# This script performs end-to-end testing and cleanup for applications
# in a Kubernetes environment using Cozystack.

# Source required modules
for file in ./modules/*.sh; do
    if [[ ! -f "$file" ]]; then
        echo "Error: Required module $file not found" >&2
        exit 1
    fi
    source "$file"
done

# Constants
ROOT_NS="tenant-root"
TEST_TENANT="tenant-e2e"

# Function to run tests
function test() {
    # ... (implementation with proper error handling)
}

# Function to clean up resources
function clean() {
    # ... (implementation with proper error handling)
}

# Main script logic
case "$1" in
    test)
        echo -e "${YELLOW}Running tests...${RESET}"
        if test; then
            echo -e "${GREEN}Tests completed successfully${RESET}"
            exit 0
        else
            echo -e "${RED}Tests failed${RESET}"
            exit 1
        fi
        ;;
    clean)
        echo -e "${YELLOW}Cleaning up...${RESET}"
        if clean; then
            echo -e "${GREEN}Cleanup completed successfully${RESET}"
            exit 0
        else
            echo -e "${RED}Cleanup failed${RESET}"
            exit 1
        fi
        ;;
    *)
        echo -e "${RED}Usage: $0 {test|clean}${RESET}"
        exit 1
        ;;
esac

These changes would significantly improve the script's structure, readability, and robustness.

🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

[warning] 2-2: ShellCheck can't follow non-constant source. Use a directive to specify location.

(SC1090)

⚠️ Potential issue

Enhance script security

The script uses environment variables and executes kubectl commands with potentially user-supplied input, which could lead to security vulnerabilities if not properly handled.

Consider implementing the following security measures:

  1. Sanitize and validate all user inputs and environment variables before use.
  2. Use kubectl --namespace="$ROOT_NS" instead of -n $ROOT_NS to prevent potential command injection.
  3. Consider using Kubernetes RBAC to limit the permissions of the service account used by this script.

Example of input validation:

function validate_namespace() {
    local ns="$1"
    if [[ ! "$ns" =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ ]]; then
        echo "Invalid namespace: $ns" >&2
        return 1
    fi
}

# Usage
if ! validate_namespace "$ROOT_NS"; then
    exit 1
fi

Implement similar validation for other user inputs and environment variables to enhance the script's security.

🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

[warning] 2-2: ShellCheck can't follow non-constant source. Use a directive to specify location.

(SC1090)

32 changes: 32 additions & 0 deletions hack/modules/check_helmrelease_status.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/bin/bash

source ./modules/colors.sh
Comment on lines +1 to +3
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

Consider using a more robust path for sourcing colors.sh

The current method of sourcing colors.sh assumes a specific directory structure, which might not be portable across different environments or when the script is called from different locations.

Consider using a more robust method to source the colors.sh file. Here's an example:

-source ./modules/colors.sh
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+source "${SCRIPT_DIR}/colors.sh"

This change ensures that the colors.sh file is sourced from the same directory as the current script, regardless of where it's called from.

📝 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
#!/bin/bash
source ./modules/colors.sh
#!/bin/bash
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
source "${SCRIPT_DIR}/colors.sh"


function check_helmrelease_status() {
local release_name="$1"
local namespace="$2"
local timeout=300 # Timeout in seconds
local interval=5 # Interval between checks in seconds
local elapsed=0
Comment on lines +5 to +10
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

Consider making timeout and interval configurable

The timeout and interval values are currently hardcoded, which might limit the script's flexibility in different environments or for different releases.

Consider making these values configurable by accepting them as optional parameters with default values. Here's an example implementation:

 function check_helmrelease_status() {
     local release_name="$1"
     local namespace="$2"
-    local timeout=300  # Timeout in seconds
-    local interval=5   # Interval between checks in seconds
+    local timeout="${3:-300}"  # Timeout in seconds, default 300
+    local interval="${4:-5}"   # Interval between checks in seconds, default 5
     local elapsed=0

This change allows users to optionally specify custom timeout and interval values when calling the function, while maintaining the current defaults if not specified.

📝 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
function check_helmrelease_status() {
local release_name="$1"
local namespace="$2"
local timeout=300 # Timeout in seconds
local interval=5 # Interval between checks in seconds
local elapsed=0
function check_helmrelease_status() {
local release_name="$1"
local namespace="$2"
local timeout="${3:-300}" # Timeout in seconds, default 300
local interval="${4:-5}" # Interval between checks in seconds, default 5
local elapsed=0


while [[ $elapsed -lt $timeout ]]; do
Copy link
Member

@kvaps kvaps Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't it be simpler to use kubectl wait --for=condition=ready?

local status_output
status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json | jq -r '.status.conditions[-1].reason')

if [[ "$status_output" == "InstallSucceeded" ]]; then
echo -e "${GREEN}Helm release '$release_name' is ready.${RESET}"
return 0
elif [[ "$status_output" == "InstallFailed" ]]; then
echo -e "${RED}Helm release '$release_name': InstallFailed${RESET}"
exit 1
else
echo -e "${YELLOW}Helm release '$release_name' is not ready. Current status: $status_output${RESET}"
fi

sleep "$interval"
elapsed=$((elapsed + interval))
done
Comment on lines +12 to +28
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

Improve error handling and provide more detailed failure information

The current implementation doesn't handle potential errors from the kubectl command, and it provides limited information in case of failure.

Consider adding error handling for the kubectl command and providing more detailed information in case of failure. Here's an example implementation:

     while [[ $elapsed -lt $timeout ]]; do
         local status_output
-        status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json | jq -r '.status.conditions[-1].reason')
+        if ! status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json 2>&1); then
+            echo -e "${RED}Error retrieving status for '$release_name': $status_output${RESET}"
+            exit 1
+        fi
+        
+        status_reason=$(echo "$status_output" | jq -r '.status.conditions[-1].reason')
+        status_message=$(echo "$status_output" | jq -r '.status.conditions[-1].message')
 
-        if [[ "$status_output" == "InstallSucceeded" ]]; then
+        if [[ "$status_reason" == "InstallSucceeded" ]]; then
             echo -e "${GREEN}Helm release '$release_name' is ready.${RESET}"
             return 0
-        elif [[ "$status_output" == "InstallFailed" ]]; then
-          echo -e "${RED}Helm release '$release_name': InstallFailed${RESET}"
+        elif [[ "$status_reason" == "InstallFailed" ]]; then
+          echo -e "${RED}Helm release '$release_name': InstallFailed${RESET}"
+          echo -e "${RED}Failure reason: $status_message${RESET}"
           exit 1
         else
-            echo -e "${YELLOW}Helm release '$release_name' is not ready. Current status: $status_output${RESET}"
+            echo -e "${YELLOW}Helm release '$release_name' is not ready. Current status: $status_reason${RESET}"
+            echo -e "${YELLOW}Status message: $status_message${RESET}"
         fi

These changes add error handling for the kubectl command and provide more detailed information about the status, including the status message, which can be helpful for debugging.

📝 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
while [[ $elapsed -lt $timeout ]]; do
local status_output
status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json | jq -r '.status.conditions[-1].reason')
if [[ "$status_output" == "InstallSucceeded" ]]; then
echo -e "${GREEN}Helm release '$release_name' is ready.${RESET}"
return 0
elif [[ "$status_output" == "InstallFailed" ]]; then
echo -e "${RED}Helm release '$release_name': InstallFailed${RESET}"
exit 1
else
echo -e "${YELLOW}Helm release '$release_name' is not ready. Current status: $status_output${RESET}"
fi
sleep "$interval"
elapsed=$((elapsed + interval))
done
while [[ $elapsed -lt $timeout ]]; do
local status_output
if ! status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json 2>&1); then
echo -e "${RED}Error retrieving status for '$release_name': $status_output${RESET}"
exit 1
fi
status_reason=$(echo "$status_output" | jq -r '.status.conditions[-1].reason')
status_message=$(echo "$status_output" | jq -r '.status.conditions[-1].message')
if [[ "$status_reason" == "InstallSucceeded" ]]; then
echo -e "${GREEN}Helm release '$release_name' is ready.${RESET}"
return 0
elif [[ "$status_reason" == "InstallFailed" ]]; then
echo -e "${RED}Helm release '$release_name': InstallFailed${RESET}"
echo -e "${RED}Failure reason: $status_message${RESET}"
exit 1
else
echo -e "${YELLOW}Helm release '$release_name' is not ready. Current status: $status_reason${RESET}"
echo -e "${YELLOW}Status message: $status_message${RESET}"
fi
sleep "$interval"
elapsed=$((elapsed + interval))
done


echo -e "${RED}Timeout reached. Helm release '$release_name' is still not ready after $timeout seconds.${RESET}"
exit 1
}
Comment on lines +30 to +32
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 timeout error message with more information

While the current timeout error message is clear, it could be more informative by including the last known status of the release.

Consider modifying the timeout error message to include the last known status of the release. Here's an example implementation:

-    echo -e "${RED}Timeout reached. Helm release '$release_name' is still not ready after $timeout seconds.${RESET}"
+    echo -e "${RED}Timeout reached. Helm release '$release_name' is still not ready after $timeout seconds.${RESET}"
+    echo -e "${RED}Last known status: $status_reason${RESET}"
+    echo -e "${RED}Last status message: $status_message${RESET}"
     exit 1

This change provides more context about the state of the release when the timeout occurs, which can be helpful for debugging and understanding why the release didn't become ready in time.

Committable suggestion was skipped due to low confidence.

6 changes: 6 additions & 0 deletions hack/modules/colors.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/bash

RED='\033[0;31m'
GREEN='\033[0;32m'
RESET='\033[0m'
YELLOW='\033[0;33m'
6 changes: 6 additions & 0 deletions hack/modules/ignored_charts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
tenant
http-cache
mysql
rabbitmq
virtual-machine
vpn
66 changes: 66 additions & 0 deletions hack/modules/install_all_apps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#!/bin/bash

source ./modules/colors.sh
Comment on lines +1 to +3
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

Add sourcing for required scripts

The script is missing the sourcing of required scripts for install_helmrelease and check_helmrelease_status functions. Add the following lines after sourcing the colors module:

source ./hack/modules/install_chart.sh
source ./hack/modules/check_helmrelease_status.sh

This ensures that all required functions are available before they are called in the script.


# Function to load ignored charts from a file
function load_ignored_charts() {
local ignore_file="$1"
local ignored_charts=()

if [[ -f "$ignore_file" ]]; then
while IFS= read -r chart; do
ignored_charts+=("$chart")
done < "$ignore_file"
else
echo "Ignore file not found: $ignore_file"
fi

# Return the array of ignored charts
echo "${ignored_charts[@]}"
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

Use 'printf' instead of 'echo' to safely handle array output

Using echo to output array elements can cause word splitting and globbing issues, especially if any chart names contain spaces or special characters. It's safer to use printf with a null character as a delimiter.

Modify the function to use printf:

-    echo "${ignored_charts[@]}"
+    printf '%s\0' "${ignored_charts[@]}"

This change ensures that the array elements are outputted correctly without unintended splitting.

📝 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
echo "${ignored_charts[@]}"
printf '%s\0' "${ignored_charts[@]}"

}
Comment on lines +6 to +20
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

Use 'printf' for safer array output

To safely handle array output, especially when chart names might contain spaces or special characters, replace the echo command with printf:

-    echo "${ignored_charts[@]}"
+    printf '%s\0' "${ignored_charts[@]}"

This change ensures that array elements are outputted correctly without unintended splitting.

📝 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
function load_ignored_charts() {
local ignore_file="$1"
local ignored_charts=()
if [[ -f "$ignore_file" ]]; then
while IFS= read -r chart; do
ignored_charts+=("$chart")
done < "$ignore_file"
else
echo "Ignore file not found: $ignore_file"
fi
# Return the array of ignored charts
echo "${ignored_charts[@]}"
}
function load_ignored_charts() {
local ignore_file="$1"
local ignored_charts=()
if [[ -f "$ignore_file" ]]; then
while IFS= read -r chart; do
ignored_charts+=("$chart")
done < "$ignore_file"
else
echo "Ignore file not found: $ignore_file"
fi
# Return the array of ignored charts
printf '%s\0' "${ignored_charts[@]}"
}


# Function to check if a chart is in the ignored list
function is_chart_ignored() {
local chart_name="$1"
shift
local ignored_charts=("$@")

for ignored_chart in "${ignored_charts[@]}"; do
if [[ "$ignored_chart" == "$chart_name" ]]; then
return 0
fi
done
return 1
}

function install_all_apps() {
local charts_dir="$1"
local namespace="$2"
local repo_name="$3"
local repo_ns="$4"

local ignore_file="./modules/ignored_charts"
local ignored_charts
ignored_charts=($(load_ignored_charts "$ignore_file"))
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

Use 'mapfile' for safer array assignment

To safely read the array elements and avoid potential word splitting issues, use mapfile with process substitution:

-    ignored_charts=($(load_ignored_charts "$ignore_file"))
+    mapfile -d '' -t ignored_charts < <(load_ignored_charts "$ignore_file")

This change reads the null-delimited output from load_ignored_charts into the ignored_charts array safely.

📝 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
ignored_charts=($(load_ignored_charts "$ignore_file"))
mapfile -d '' -t ignored_charts < <(load_ignored_charts "$ignore_file")
🧰 Tools
🪛 Shellcheck

[warning] 44-44: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


for chart_path in "$charts_dir"/*; do
if [[ -d "$chart_path" ]]; then
local chart_name
chart_name=$(basename "$chart_path")
# Check if the chart is in the ignored list
if is_chart_ignored "$chart_name" "${ignored_charts[@]}"; then
echo "Skipping chart: $chart_name (listed in ignored charts)"
continue
fi

release_name="$chart_name-e2e"
echo "Installing release: $release_name"
install_helmrelease "$release_name" "$namespace" "$chart_name" "$repo_name" "$repo_ns"

echo "Checking status for HelmRelease: $release_name"
check_helmrelease_status "$release_name" "$namespace"
Comment on lines +58 to +61
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

Add error handling for external function calls

Add error handling for the install_helmrelease and check_helmrelease_status function calls to ensure the script can handle potential failures gracefully:

 echo "Installing release: $release_name"
-install_helmrelease "$release_name" "$namespace" "$chart_name" "$repo_name" "$repo_ns"
+if ! install_helmrelease "$release_name" "$namespace" "$chart_name" "$repo_name" "$repo_ns"; then
+    echo "Failed to install release: $release_name"
+    continue
+fi

 echo "Checking status for HelmRelease: $release_name"
-check_helmrelease_status "$release_name" "$namespace"
+if ! check_helmrelease_status "$release_name" "$namespace"; then
+    echo "HelmRelease status check failed for: $release_name"
+fi

This change ensures that the script continues to the next chart if installation fails and reports any status check failures.

📝 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
install_helmrelease "$release_name" "$namespace" "$chart_name" "$repo_name" "$repo_ns"
echo "Checking status for HelmRelease: $release_name"
check_helmrelease_status "$release_name" "$namespace"
echo "Installing release: $release_name"
if ! install_helmrelease "$release_name" "$namespace" "$chart_name" "$repo_name" "$repo_ns"; then
echo "Failed to install release: $release_name"
continue
fi
echo "Checking status for HelmRelease: $release_name"
if ! check_helmrelease_status "$release_name" "$namespace"; then
echo "HelmRelease status check failed for: $release_name"
fi

else
echo "$chart_path is not a directory. Skipping."
fi
done
}
60 changes: 60 additions & 0 deletions hack/modules/install_chart.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#!/bin/bash

source ./modules/colors.sh

function install_helmrelease() {
local release_name="$1"
local namespace="$2"
local chart_path="$3"
local repo_name="$4"
local repo_ns="$5"
local values_file="$6"

if [[ -z "$release_name" ]]; then
echo -e "${RED}Error: Release name is required.${RESET}"
exit 1
fi

if [[ -z "$namespace" ]]; then
echo -e "${RED}Error: Namespace name is required.${RESET}"
exit 1
fi

if [[ -z "$chart_path" ]]; then
echo -e "${RED}Error: Chart path name is required.${RESET}"
exit 1
fi

local helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)

Comment on lines +28 to +29
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

Separate variable declaration and assignment for error handling

To improve error handling and follow best practices, it's recommended to separate the declaration and assignment of the helmrelease_file variable. This allows for checking if mktemp succeeded and handling any potential errors.

Apply this change to address the issue:

-local helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)
+local helmrelease_file
+helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)
+if [[ $? -ne 0 ]]; then
+    echo -e "${RED}Error: Failed to create temporary file.${RESET}"
+    exit 1
+fi

This change allows you to check if mktemp succeeded and handle the error appropriately.

📝 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
local helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)
local helmrelease_file
helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)
if [[ $? -ne 0 ]]; then
echo -e "${RED}Error: Failed to create temporary file.${RESET}"
exit 1
fi
🧰 Tools
🪛 Shellcheck

[warning] 28-28: Declare and assign separately to avoid masking return values.

(SC2155)

{
echo "apiVersion: helm.toolkit.fluxcd.io/v2"
echo "kind: HelmRelease"
echo "metadata:"
echo " labels:"
echo " cozystack.io/ui: \"true\""
echo " name: \"$release_name\""
echo " namespace: \"$namespace\""
echo "spec:"
echo " chart:"
echo " spec:"
echo " chart: \"$chart_path\""
echo " reconcileStrategy: Revision"
echo " sourceRef:"
echo " kind: HelmRepository"
echo " name: \"$repo_name\""
echo " namespace: \"$repo_ns\""
echo " version: '*'"
echo " interval: 1m0s"
echo " timeout: 5m0s"

if [[ -n "$values_file" && -f "$values_file" ]]; then
echo " values:"
cat "$values_file" | sed 's/^/ /'
fi
} > "$helmrelease_file"

kubectl apply -f "$helmrelease_file"

rm -f "$helmrelease_file"
}
Comment on lines +57 to +60
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

Add error handling for kubectl command

While the YAML application and cleanup are correctly implemented, it's important to handle potential errors when applying the YAML with kubectl.

Implement error handling for the kubectl command as follows:

-kubectl apply -f "$helmrelease_file"
+if ! kubectl apply -f "$helmrelease_file"; then
+    echo -e "${RED}Error: Failed to apply HelmRelease.${RESET}"
+    rm -f "$helmrelease_file"
+    exit 1
+fi

rm -f "$helmrelease_file"

This change ensures that any errors during the kubectl apply operation are caught and reported, providing better feedback to the user and maintaining the script's robustness.

📝 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
kubectl apply -f "$helmrelease_file"
rm -f "$helmrelease_file"
}
if ! kubectl apply -f "$helmrelease_file"; then
echo -e "${RED}Error: Failed to apply HelmRelease.${RESET}"
rm -f "$helmrelease_file"
exit 1
fi
rm -f "$helmrelease_file"
}

11 changes: 11 additions & 0 deletions hack/modules/install_tenant.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/bash

function install_tenant (){
local release_name="$1"
local namespace="$2"
local values_file="${3:-tenant.yaml}"
local repo_name="cozystack-apps"
local repo_ns="cozy-public"

install_helmrelease "$release_name" "$namespace" "tenant" "$repo_name" "$repo_ns" "$values_file"
}
2 changes: 1 addition & 1 deletion hack/pre-checks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ RED='\033[31m'
RESET='\033[0m'

check-yq-version() {
current_version=$(yq -V | grep -oP 'v[0-9]+\.[0-9]+\.[0-9]+')
current_version=$(yq -V | awk '$(NF-1) == "version" {print $NF}')
if [ -z "$current_version" ]; then
echo "yq is not installed or version cannot be determined."
exit 1
Expand Down
6 changes: 6 additions & 0 deletions hack/values/tenant.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
host: ""
etcd: false
monitoring: true
ingress: false
seaweedfs: true
isolated: true
2 changes: 1 addition & 1 deletion packages/apps/ferretdb/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.4.0
version: 0.4.1

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
Expand Down
7 changes: 5 additions & 2 deletions packages/apps/ferretdb/templates/init-script.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ stringData:
init.sh: |
#!/bin/bash
set -e

until pg_isready ; do sleep 5; done

echo "== create users"
{{- if .Values.users }}
psql -v ON_ERROR_STOP=1 <<\EOT
Expand All @@ -60,7 +63,7 @@ stringData:
DROP USER $user;
EOT
done

echo "== create roles"
psql -v ON_ERROR_STOP=1 --echo-all <<\EOT
SELECT 'CREATE ROLE app_admin NOINHERIT;'
Expand All @@ -80,7 +83,7 @@ stringData:
FOR schema_record IN SELECT schema_name FROM information_schema.schemata WHERE schema_name NOT IN ('pg_catalog', 'information_schema') LOOP
-- Changing Schema Ownership
EXECUTE format('ALTER SCHEMA %I OWNER TO %I', schema_record.schema_name, 'app_admin');

-- Add rights for the admin role
EXECUTE format('GRANT ALL ON SCHEMA %I TO %I', schema_record.schema_name, 'app_admin');
EXECUTE format('GRANT ALL ON ALL TABLES IN SCHEMA %I TO %I', schema_record.schema_name, 'app_admin');
Expand Down
3 changes: 2 additions & 1 deletion packages/apps/versions_map
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ ferretdb 0.1.0 4ffa8615
ferretdb 0.1.1 5ca8823
ferretdb 0.2.0 adaf603
ferretdb 0.3.0 aa2f553
ferretdb 0.4.0 HEAD
ferretdb 0.4.0 def2eb0f
ferretdb 0.4.1 HEAD
Comment on lines +12 to +13
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

Commit hash def2eb0f for ferretdb version 0.4.0 not found.

The specified commit hash def2eb0f for ferretdb version 0.4.0 does not exist in the repository. Please verify and update the commit hash accordingly.

🔗 Analysis chain

LGTM! Verify commit hashes for ferretdb versions.

The changes to the ferretdb package versions look good. Version 0.4.0 has been updated with a specific commit hash, and a new version 0.4.1 has been added pointing to HEAD.

To ensure the accuracy of these changes, please run the following script to verify the commit hashes:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify commit hashes for ferretdb versions

# Test: Check if the commit hash for version 0.4.0 exists
git rev-parse --quiet --verify def2eb0f

# Test: Check if HEAD is different from the 0.4.0 commit
if [ "$(git rev-parse HEAD)" != "$(git rev-parse def2eb0f)" ]; then
    echo "HEAD is different from 0.4.0 commit, which is correct."
else
    echo "Warning: HEAD is the same as 0.4.0 commit. Please verify if this is intended."
fi

Length of output: 462

http-cache 0.1.0 a956713
http-cache 0.2.0 5ca8823
http-cache 0.3.0 fab5940
Expand Down
Loading