Add tests refactor #451
Add tests refactor #451
Conversation
dosubot
bot
added
the
size:L
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several changes across multiple files. It updates the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
hack/values/tenant.yaml (1)
1-6: Consider expanding the configuration and documenting its purpose.The current configuration provides a basic setup, but it might be incomplete for a full application deployment. Given its location in the 'hack/values/' directory, it appears this might be used for development or testing purposes.
Consider the following suggestions:
- Document the specific purpose of this configuration file (e.g., local development, staging environment, etc.).
- Evaluate if additional configuration options are needed, such as:
- Database connection settings
- API endpoints or service URLs
- Environment-specific variables
- Logging levels or configurations
- If this is indeed a partial configuration, consider adding a comment in the file explaining where to find the complete set of configuration options.
packages/core/testing/Makefile (3)
4-4: LGTM! Consider adding a filter for valid test targets.The
TESTING_APPSvariable dynamically collects application names, which is a flexible approach. However, you might want to add a filter to ensure only valid test targets are included.Consider modifying the command to filter for specific criteria, e.g.:
TESTING_APPS := $(shell find ../../apps -maxdepth 1 -mindepth 1 -type d -name '*-app' | awk -F/ '{print $$NF}')This assumes that valid test targets have names ending with "-app". Adjust the filter as needed for your specific naming conventions.
36-38: LGTM! Consider optimizing the copied files.The
copy-hack-dirtarget efficiently copies the hack directory into the Kubernetes deployment. This is a good approach for ensuring necessary scripts are available in the test environment.To optimize, consider copying only the required files instead of the entire hack directory. For example:
copy-hack-dir: tar -C ../../../hack -cf- e2e.application.sh | kubectl exec -i -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- tar -xf- -C /hack/This assumes that only
e2e.application.shis needed. Adjust the file list as necessary for your specific requirements.
43-49: LGTM! Consider pre-installing tools and parameterizing the test script.The new
test-applicationstarget provides a flexible way to test multiple applications. The approach of iterating overTESTING_APPSis good for adaptability.Consider the following improvements:
- Pre-install
yqandjqin the Docker image to speed up test execution.- Parameterize the test script path to make it more flexible:
TEST_SCRIPT ?= /hack/e2e.application.sh test-applications: wait-for-sandbox copy-hack-dir for app in $(TESTING_APPS); do \ echo "Running tests for $${app}"; \ kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- bash -c "$(TEST_SCRIPT) $${app}"; \ done kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- bash -c "kubectl get hr -A | grep -v 'True'"This allows you to easily change the test script if needed.
hack/e2e.application.sh (3)
89-116: Enhance error handling and output incheck_helmrelease_status.The function is well-structured with a timeout mechanism, but there's room for improvement in error handling and output.
Consider the following enhancements:
- Add error handling for the
kubectl getcommand:- status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json | jq -r '.status.conditions[-1].reason') + status_output=$(kubectl get helmrelease "$release_name" -n "$namespace" -o json | jq -r '.status.conditions[-1].reason') || { echo -e "${RED}Failed to get HelmRelease status${RESET}"; return 1; }
- Provide more context in the timeout message:
- 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' in namespace '$namespace' is still not ready after $timeout seconds.${RESET}"These changes will make the function more robust and provide clearer output for debugging purposes.
118-127: LGTM: Main script logic is well-structured.The main script logic correctly checks for the required chart name argument and uses the defined functions to install and check the status of the tenant.
Consider adding a check to ensure the
ROOT_NSnamespace exists before installing the tenant:if ! kubectl get namespace "$ROOT_NS" &>/dev/null; then echo -e "${RED}Error: Namespace $ROOT_NS does not exist.${RESET}" exit 1 fiThis additional check would prevent potential errors if the required namespace is not set up.
129-135: LGTM: Final installation and status check are properly implemented.The script correctly uses the defined functions to install the specified chart and check its status.
Consider adding error handling for the
install_helmreleasefunction call:-install_helmrelease "$release_name" "$TEST_TENANT" "$chart_name" "$repo_name" "$repo_ns" +if ! install_helmrelease "$release_name" "$TEST_TENANT" "$chart_name" "$repo_name" "$repo_ns"; then + echo -e "${RED}Failed to install HelmRelease for $chart_name${RESET}" + exit 1 +fiThis change would ensure that the script exits with an error if the installation fails, preventing potential issues in subsequent steps.
packages/apps/ferretdb/templates/init-script.yaml (2)
Line range hint
95-120: LGTM: Event trigger for automatic privilege grantingThe implementation of the event trigger
trigger_auto_grantis well-done. It ensures that theapp_adminrole automatically receives the necessary privileges on newly created schemas, maintaining consistency with the initial setup.Suggestion for improvement:
Consider adding error handling within theauto_grant_schema_privileges()function to gracefully handle any failures during privilege assignment.You could add error handling like this:
CREATE OR REPLACE FUNCTION auto_grant_schema_privileges() RETURNS event_trigger LANGUAGE plpgsql AS $$ DECLARE obj record; BEGIN FOR obj IN SELECT * FROM pg_event_trigger_ddl_commands() WHERE command_tag = 'CREATE SCHEMA' LOOP BEGIN -- Your existing code here EXCEPTION WHEN OTHERS THEN RAISE WARNING 'Failed to grant privileges on schema %: %', obj.object_identity, SQLERRM; END; END LOOP; END; $$;This will ensure the function continues processing other schemas even if one fails.
Line range hint
122-127: Consider a more granular approach to role assignmentThe current implementation grants the
app_adminrole to all users defined in.Values.users. This approach might be overly permissive, as it doesn't differentiate between different types of users (e.g., regular users vs. administrators).Suggestions for improvement:
- Introduce a flag in the user definition to indicate whether they should receive admin privileges.
- Create separate roles for different levels of access and assign them based on user attributes.
Example of a more granular approach:
users: user1: isAdmin: true user2: isAdmin: falseThen in the SQL:
{{- range $user, $u := $.Values.users }} {{- if $u.isAdmin }} GRANT app_admin TO {{ $user }}; {{- else }} GRANT app_user TO {{ $user }}; -- assuming you create an app_user role with fewer privileges {{- end }} {{- end }}This would allow for more fine-grained control over user permissions.
Would you like assistance in implementing this more granular role assignment approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .pre-commit-config.yaml (1 hunks)
- hack/e2e.application.sh (1 hunks)
- hack/values/tenant.yaml (1 hunks)
- packages/apps/ferretdb/templates/init-script.yaml (2 hunks)
- packages/core/testing/Makefile (2 hunks)
🧰 Additional context used
🪛 Shellcheck
hack/e2e.application.sh
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (13)
hack/values/tenant.yaml (5)
1-1: Verify the intention of the empty host value.The
hostis set to an empty string. While this might provide flexibility, it could potentially lead to security or routing issues if not handled correctly in the application code. Please confirm if this is the intended configuration.
3-3: Monitoring is enabled. Verify the monitoring setup.It's great to see that monitoring is enabled. This is crucial for maintaining visibility into the application's performance and health.
Please ensure that the monitoring system is properly configured and integrated with your observability stack.
4-4: Verify the intention of disabled ingress and document access method.Ingress is set to
false, indicating that Kubernetes Ingress is not being used for external access. Please confirm if this is intentional and consider documenting the intended method of accessing the application in this configuration.
5-5: SeaweedFS is enabled. Verify the setup and dependencies.The configuration shows that SeaweedFS is enabled, which is good for distributed file storage.
Please ensure that:
- All necessary SeaweedFS components are properly configured and deployed.
- The application code is set up to interact correctly with SeaweedFS.
- Any required network policies or security configurations for SeaweedFS are in place.
6-6: Clarify the implications of the 'isolated' setting.The
isolatedflag is set totrue. The implications of this setting may vary depending on how it's used in the application code.Could you please provide more context on:
- What exactly does the 'isolated' setting control in the application?
- Are there any specific security measures or resource limitations that are enforced when this is set to true?
- How does this setting affect the application's interaction with other services or components?
.pre-commit-config.yaml (2)
15-16: LGTM: Improved indentation formarkdownlinthook.The indentation adjustment for the
markdownlinthook'sargsimproves the readability and consistency of the YAML file. This change adheres to YAML specification and pre-commit configuration standards.
15-25: Overall, these changes enhance the pre-commit configuration.The indentation fix and the addition of the
gen-versions-maphook contribute to better code quality and version management. These modifications align well with the PR's objective of adding tests and refactoring. The new hook, in particular, should help maintain version consistency, which is crucial for effective testing across the project.To ensure these changes are effective, let's verify the pre-commit hook functionality:
packages/core/testing/Makefile (1)
51-52: Clarify the purpose and implementation of the test-result target.The
test-resulttarget is currently empty and its purpose is unclear. It seems to overlap with thetest-applicationstarget.Please clarify:
- What is the intended functionality of this target?
- How does it differ from
test-applications?- If it's meant to be implemented later, consider adding a TODO comment.
If this target is not needed, consider removing it to avoid confusion.
hack/e2e.application.sh (3)
1-10: LGTM: Good setup with color codes and constants.The script starts with a proper shebang, defines color codes for better output readability, and uses constants for important values. This is a good practice for maintainability and consistency.
79-87: LGTM: Well-designed wrapper function.The
install_tenantfunction provides a convenient wrapper aroundinstall_helmreleasewith sensible defaults. This simplifies the process of installing a tenant and promotes code reuse.
1-135: Overall assessment: Well-structured script with room for improvement in error handling.The
e2e.application.shscript provides a solid foundation for managing Helm releases in a Kubernetes environment. It includes functions for cleaning up, installing, and checking the status of releases. The use of constants, color-coded output, and modular functions contributes to its readability and maintainability.However, there are several areas where error handling and robustness could be improved:
- The
cleanfunction should actually check the deletion status.- The
install_helmreleasefunction could benefit from better error handling, especially for temporary file creation and kubectl apply.- The
check_helmrelease_statusfunction could provide more context in error messages.- The main script logic could include additional checks, such as verifying the existence of the required namespace.
Addressing these points will make the script more resilient to potential issues and easier to debug when problems occur.
🧰 Tools
🪛 Shellcheck
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
packages/apps/ferretdb/templates/init-script.yaml (2)
Line range hint
66-71: LGTM: Creation of app_admin roleThe creation of the
app_adminrole is implemented correctly. Using NOINHERIT is a good practice for admin roles, and the comment helps with role management.
Line range hint
73-93: Review security implications of broad privilegesThe
app_adminrole is granted very extensive privileges across all user-defined schemas, including ownership of theappdatabase. While this approach ensures the role has full access, it may violate the principle of least privilege.Consider the following:
- Review if
app_admintruly needs ownership of all schemas and objects.- Evaluate if more granular permissions would be sufficient for your use case.
- Ensure that access to this role is strictly controlled, given its extensive privileges.
The implementation itself is efficient, using a DO block to loop through schemas and set both current and default privileges.
To assist in evaluating the impact, you may want to run the following command to list all schemas in the database:
This will help in understanding the scope of schemas affected by these privilege grants.
.pre-commit-config.yaml
Outdated
| - 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the new gen-versions-map hook for robustness and efficiency.
The addition of the gen-versions-map hook is a good practice for maintaining version consistency. However, consider the following improvements:
- Limit the hook to specific file types or paths to improve efficiency.
- Add error handling for the
cdcommand and themakeexecution. - Consider using
always_run: trueinstead oftypes: [file]if this should run regardless of file changes.
Here's a suggested improvement:
- 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 || exit 1'
language: system
always_run: true
pass_filenames: false
description: Run the script and fail if it generates changes or encounters an errorThis change ensures the hook runs on every commit, handles errors properly, and provides clearer feedback.
| 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 | ||
| } |
There was a problem hiding this comment.
Improve error handling in the clean function.
The current implementation always reports success, regardless of whether the deletion was actually successful. This could lead to false positives.
Consider modifying the function to actually check the deletion status:
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
}📝 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.
| 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 | |
| } |
| 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) | ||
| { | ||
| 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" | ||
| } |
There was a problem hiding this comment.
Improve error handling in install_helmrelease function.
The function is well-structured, but there's room for improvement in error handling.
- Address the Shellcheck warning by separating the declaration and assignment of
helmrelease_file:
- local helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml)
+ local helmrelease_file
+ helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml) || { echo "Failed to create temp file"; exit 1; }- Add error handling for the
kubectl applycommand:
- kubectl apply -f "$helmrelease_file"
+ if ! kubectl apply -f "$helmrelease_file"; then
+ echo -e "${RED}Failed to apply HelmRelease${RESET}"
+ rm -f "$helmrelease_file"
+ return 1
+ fiThese changes will make the function more robust and prevent potential issues from silent 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.
| 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) | |
| { | |
| 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" | |
| } | |
| 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 | |
| helmrelease_file=$(mktemp /tmp/HelmRelease.XXXXXX.yaml) || { echo "Failed to create temp file"; exit 1; } | |
| { | |
| 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" | |
| if ! kubectl apply -f "$helmrelease_file"; then | |
| echo -e "${RED}Failed to apply HelmRelease${RESET}" | |
| rm -f "$helmrelease_file" | |
| return 1 | |
| fi | |
| rm -f "$helmrelease_file" | |
| } |
🧰 Tools
🪛 Shellcheck
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/testing/images/e2e-sandbox/Dockerfile (2)
16-16: LGTM! Consider version parameterization and checksum verification.The installation of
yqis correct and follows good practices. To further improve:
Consider using an ARG for the yq version to make it easily updatable:
ARG YQ_VERSION=4.44.3 RUN wget https://github.com/mikefarah/yq/releases/download/v${YQ_VERSION}/yq_linux_amd64 -O /usr/local/bin/yq && chmod +x /usr/local/bin/yqAdd a checksum verification step for enhanced security:
ARG YQ_VERSION=4.44.3 ARG YQ_CHECKSUM=7d3d3b0e5a6c8a4f29e9c7972df7f698c3af94f80e3a8df1c26b30d5d3fb2c0e RUN wget https://github.com/mikefarah/yq/releases/download/v${YQ_VERSION}/yq_linux_amd64 -O /usr/local/bin/yq && \ echo "${YQ_CHECKSUM} /usr/local/bin/yq" | sha256sum -c && \ chmod +x /usr/local/bin/yq
16-17: Overall improvement to e2e-sandbox image capabilities.The addition of
yqandjqtools enhances the image's ability to process YAML and JSON files, which is likely beneficial for e2e testing scenarios. These changes align well with the broader updates in the repository, particularly the enhancements to testing capabilities in the Kubernetes environment.To further improve the Dockerfile:
- Consider grouping all tool version declarations at the beginning of the file using ARG instructions.
- Optimize the installation commands to reduce the number of layers and the final image size.
These changes will make the Dockerfile more maintainable and efficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/testing/Makefile (2 hunks)
- packages/core/testing/images/e2e-sandbox/Dockerfile (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/testing/Makefile
🧰 Additional context used
| @@ -13,3 +13,5 @@ RUN curl -LO "https://dl.k8s.io/release/v${KUBECTL_VERSION}/bin/linux/amd64/kube | |||
| && chmod +x kubectl \ | |||
| && mv kubectl /usr/local/bin/kubectl | |||
| RUN curl -sSL https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 | bash -s - --version "v${HELM_VERSION}" | |||
| RUN wget https://github.com/mikefarah/yq/releases/download/v4.44.3/yq_linux_amd64 -O /usr/local/bin/yq && chmod +x /usr/local/bin/yq | |||
| RUN apt-get install jq -y | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize jq installation for better Dockerfile practices.
While the installation of jq is functional, consider the following improvements:
-
Combine with previous apt-get commands to reduce layers:
RUN apt-get update && \ apt-get install -y \ genisoimage \ qemu-kvm \ qemu-utils \ iproute2 \ iptables \ wget \ xz-utils \ netcat \ curl \ jq && \ apt-get clean && \ rm -rf /var/lib/apt/lists/* -
Pin the version of jq for reproducibility:
RUN apt-get update && \ apt-get install -y jq=1.6-2.1ubuntu3 && \ apt-get clean && \ rm -rf /var/lib/apt/lists/*
These changes will result in a more efficient and reproducible Dockerfile.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/testing/Makefile (2 hunks)
- packages/core/testing/images/e2e-sandbox/Dockerfile (1 hunks)
- packages/core/testing/values.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/testing/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/testing/images/e2e-sandbox/Dockerfile
🧰 Additional context used
🔇 Additional comments (2)
packages/core/testing/Makefile (2)
4-4: VariableTESTING_APPScorrectly collects application directoriesThe command effectively gathers the names of the applications for testing by finding directories under
../../apps.
39-39: Includingcopy-hack-dirintesttarget ensures proper setupAdding
copy-hack-diras a dependency for thetesttarget guarantees that thehackdirectory is copied before tests run, preventing potential missing file issues.
| copy-hack-dir: | ||
| tar -C ../../../ -cf- hack | kubectl exec -i -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- tar -xf- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using kubectl cp to simplify directory copying
Using kubectl cp can streamline the process of copying the hack directory into the Kubernetes deployment, making the command more readable and maintainable.
Apply this diff to use kubectl cp:
copy-hack-dir:
- tar -C ../../../ -cf- hack | kubectl exec -i -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- tar -xf-
+ kubectl cp ../../../hack $(NAMESPACE)/deploy/cozystack-e2e-$(NAME):/hackNote: Ensure that the kubectl cp command works correctly in your environment and that the pod has the necessary permissions to receive files.
📝 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.
| copy-hack-dir: | |
| tar -C ../../../ -cf- hack | kubectl exec -i -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- tar -xf- | |
| copy-hack-dir: | |
| kubectl cp ../../../hack $(NAMESPACE)/deploy/cozystack-e2e-$(NAME):/hack |
| helm template -n cozy-system installer ../installer | kubectl exec -i -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- sh -c 'cat > /cozystack-installer.yaml' | ||
| kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- sh -c 'export COZYSTACK_INSTALLER_YAML=$$(cat /cozystack-installer.yaml) && /e2e.sh' | ||
| kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- sh -c 'export COZYSTACK_INSTALLER_YAML=$$(cat /cozystack-installer.yaml) && /hack/e2e.sh' |
There was a problem hiding this comment.
Avoid passing large files through environment variables
Assigning the contents of /cozystack-installer.yaml to an environment variable may lead to issues if the file size exceeds shell limits.
Consider passing the file path to the script instead. Apply this diff:
kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- sh -c \
- 'export COZYSTACK_INSTALLER_YAML=$$(cat /cozystack-installer.yaml) && /hack/e2e.sh'
+ '/hack/e2e.sh /cozystack-installer.yaml'Then, update /hack/e2e.sh to accept the installer file path as an argument and read from it internally.
This approach avoids potential issues with environment variable size limits and makes the script more flexible.
📝 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.
| kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- sh -c 'export COZYSTACK_INSTALLER_YAML=$$(cat /cozystack-installer.yaml) && /hack/e2e.sh' | |
| kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- sh -c \ | |
| '/hack/e2e.sh /cozystack-installer.yaml' |
| test-applications: wait-for-sandbox copy-hack-dir ## Run the end-to-end tests in existing sandbox for applications. | ||
| for app in $(TESTING_APPS); do \ | ||
| echo "Running tests for $${app}"; \ | ||
| kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "/hack/e2e.application.sh $${app}"; \ | ||
| done | ||
| kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "kubectl get hr -A | grep -v 'True'" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use variables for namespace and deployment name for consistency
The hardcoded namespace cozy-e2e-tests and deployment name cozystack-e2e-sandbox should be replaced with $(NAMESPACE) and cozystack-e2e-$(NAME) to maintain consistency and ease of maintenance.
Apply this diff to utilize the predefined variables:
test-applications: wait-for-sandbox copy-hack-dir ## Run the end-to-end tests in existing sandbox for applications.
for app in $(TESTING_APPS); do \
echo "Running tests for $${app}"; \
- kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "/hack/e2e.application.sh $${app}"; \
+ kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- bash -c "/hack/e2e.application.sh $${app}"; \
done
- kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "kubectl get hr -A | grep -v 'True'"
+ kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- bash -c "kubectl get hr -A | grep -v 'True'"This change improves readability and ensures that any future updates to $(NAMESPACE) or $(NAME) are automatically reflected throughout the script.
📝 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.
| test-applications: wait-for-sandbox copy-hack-dir ## Run the end-to-end tests in existing sandbox for applications. | |
| for app in $(TESTING_APPS); do \ | |
| echo "Running tests for $${app}"; \ | |
| kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "/hack/e2e.application.sh $${app}"; \ | |
| done | |
| kubectl exec -ti -n cozy-e2e-tests deploy/cozystack-e2e-sandbox -- bash -c "kubectl get hr -A | grep -v 'True'" | |
| test-applications: wait-for-sandbox copy-hack-dir ## Run the end-to-end tests in existing sandbox for applications. | |
| for app in $(TESTING_APPS); do \ | |
| echo "Running tests for $${app}"; \ | |
| kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- bash -c "/hack/e2e.application.sh $${app}"; \ | |
| done | |
| kubectl exec -ti -n $(NAMESPACE) deploy/cozystack-e2e-$(NAME) -- bash -c "kubectl get hr -A | grep -v 'True'" |
Signed-off-by: Andrei Kvapil <[email protected]>
kvaps
force-pushed
the
add-tests-refactor
branch
from
8855e73 to
95897d6
Compare
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores