Skip to content

Add make targets for local development environment setup#137

Open
PuneetPunamiya wants to merge 1 commit intoredhat-data-and-ai:mainfrom
PuneetPunamiya:adds-make-targers-for-local-setup
Open

Add make targets for local development environment setup#137
PuneetPunamiya wants to merge 1 commit intoredhat-data-and-ai:mainfrom
PuneetPunamiya:adds-make-targers-for-local-setup

Conversation

@PuneetPunamiya
Copy link
Copy Markdown
Contributor

  • Add local-dev-setup and local-dev-cleanup make targets with corresponding
    scripts to automate local development environment creation.

Add local-dev-setup and local-dev-cleanup make targets with corresponding
scripts to automate local development environment creation.

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces automated local development environment setup and cleanup through new Makefile targets and bash scripts. The scripts handle Kind cluster creation, namespace setup, local caching, Docling service management, and LocalStack resource configuration. Feedback was provided to improve the scripts' robustness by adding dependency checks, using relative paths for configuration files, removing unused variables, and ensuring the cleanup process removes the local cache directory.

CACHE_DIR="${CACHE_DIR:-tmp/cache}"
DATA_STORAGE_BUCKET="${DATA_STORAGE_BUCKET:-data-storage-bucket}"
DATA_INGESTION_BUCKET="${DATA_INGESTION_BUCKET:-data-ingestion-bucket}"
OUTPUT_RESULT_BUCKET="${OUTPUT_RESULT_BUCKET:-output-result-bucket}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The variable OUTPUT_RESULT_BUCKET is defined but not used anywhere in the script. If this bucket is intended to be part of the local setup, it should be included in the creation loop at line 71. Otherwise, it should be removed to maintain code clarity.

SQS_QUEUE_NAME="${SQS_QUEUE_NAME:-unstructured-s3-queue}"

echo "Setting up local development environment..."
echo "Note: Make sure LocalStack is running in a separate terminal:"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The script relies on several external tools (kind, kubectl, awslocal, curl). It is recommended to verify their presence at the beginning of the script to provide a clear error message to the user instead of failing later with a 'command not found' error.

Suggested change
echo "Note: Make sure LocalStack is running in a separate terminal:"
# Check dependencies
for cmd in kind kubectl awslocal curl; do
if ! command -v "$cmd" &> /dev/null; then
echo "Error: $cmd is not installed. Please install it to continue." >&2
exit 1
fi
done
echo "Note: Make sure LocalStack is running in a separate terminal:"

echo "5c. Configuring S3 bucket notification..."
awslocal s3api put-bucket-notification-configuration \
--bucket "${DATA_INGESTION_BUCKET}" \
--notification-configuration file://test/resources/unstructured/notification.json
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The path to the notification configuration file is relative to the current working directory. This makes the script fragile if executed from a directory other than the project root. Using a path relative to the script's location is more robust.

Suggested change
--notification-configuration file://test/resources/unstructured/notification.json
--notification-configuration "file://$(dirname "$0")/../test/resources/unstructured/notification.json"

fi

# Delete Kind cluster
kind delete cluster --name "${LOCAL_KIND_CLUSTER}" 2>/dev/null || true
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The cleanup script should also remove the local cache directory created by the setup script to ensure a complete teardown of the development environment.

Suggested change
kind delete cluster --name "${LOCAL_KIND_CLUSTER}" 2>/dev/null || true
kind delete cluster --name "${LOCAL_KIND_CLUSTER}" 2>/dev/null || true
# Remove local cache
rm -rf tmp/cache

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant