Skip to content

Conversation

@rjrudin
Copy link
Contributor

@rjrudin rjrudin commented Oct 6, 2025

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Copyright Validation Results
Total: 4 | Passed: 0 | Failed: 0 | Skipped: 4 | at: 2025-10-07 14:27:39 UTC | commit: 73c1ebb

⏭️ Skipped (Excluded) Files

  • Jenkinsfile
  • package-lock.json
  • package.json
  • test-app/docker-compose.yaml

✅ All files have valid copyright headers!

Also bumped up tar-fs to resolve a vulnerability.

Going to format Jenkinsfile in a subsequent PR.
@rjrudin rjrudin force-pushed the feature/24531-docker branch from 6e2d821 to 73c1ebb Compare October 7, 2025 14:27
@rjrudin rjrudin marked this pull request as ready for review October 7, 2025 14:27
Copilot AI review requested due to automatic review settings October 7, 2025 14:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR converts the Jenkins pipeline from using direct MarkLogic RPM installations to using Docker containers, streamlining the build and test environment setup.

  • Replaced manual MarkLogic RPM installation with Docker Compose-based setup
  • Updated port mappings and container configuration for consistent test environment
  • Removed obsolete Docker Compose configuration files and test stages

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
test-app/docker-compose.yaml Updated service name and consolidated port mappings for Docker-based testing
test-app/docker-compose-nightlies.yaml Removed obsolete nightly build configuration file
package.json Updated tar-fs dependency to newer version
Jenkinsfile Converted from RPM-based to Docker-based MarkLogic deployment with new helper functions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

sudo /usr/local/sbin/mladmin cleandata
cd node-client-api/test-app
MARKLOGIC_LOGS_VOLUME=/tmp MARKLOGIC_IMAGE='''+markLogicDockerImage+''' docker-compose up -d --build
sleep 60s;
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

The hardcoded 60-second sleep is a fragile approach for waiting for container readiness. Consider using a health check or polling mechanism to verify MarkLogic is actually ready before proceeding.

Suggested change
sleep 60s;
# Wait for MarkLogic to be ready by polling the HTTP endpoint
echo "Waiting for MarkLogic to become ready..."
for i in {1..30}; do
if curl -s http://localhost:8001/ > /dev/null; then
echo "MarkLogic is ready!"
break
else
echo "MarkLogic not ready yet, waiting..."
sleep 2
fi
if [ $i -eq 30 ]; then
echo "MarkLogic did not become ready in time." >&2
exit 1
fi
done

Copilot uses AI. Check for mistakes.
docker-compose down -v || true
sudo /usr/local/sbin/mladmin cleandata
cd node-client-api/test-app
MARKLOGIC_LOGS_VOLUME=/tmp MARKLOGIC_IMAGE='''+markLogicDockerImage+''' docker-compose up -d --build
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

Using /tmp for log volume could lead to log loss and potential disk space issues. Consider using a more persistent location or making this configurable.

Suggested change
MARKLOGIC_LOGS_VOLUME=/tmp MARKLOGIC_IMAGE='''+markLogicDockerImage+''' docker-compose up -d --build
MARKLOGIC_LOGS_VOLUME=$WORKSPACE/marklogic-logs MARKLOGIC_IMAGE='''+markLogicDockerImage+''' docker-compose up -d --build

Copilot uses AI. Check for mistakes.
@rjrudin rjrudin merged commit 2bc330c into develop Oct 7, 2025
2 of 3 checks passed
@rjrudin rjrudin deleted the feature/24531-docker branch October 7, 2025 14:55
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.

2 participants