Skip to content

Conversation

@torcolvin
Copy link
Collaborator

The motivation to do this was that SG_TEST_BUCKET_POOL is explicitly set by this script and I think we should move to having these default values determined by the code and jenkins could optionally overwrite them.

This is a low priority change to review.

This change is no-op, there are no functional changes to the script except not requiring setting SG_TEST_BUCKET_POOL_SIZE and
SG_TEST_BUCKET_POOL_DEBUG before you call this script. Note, these are always currently set by the jenkins pipelines anyway, so the values would pass through. So this is also not a functional change with the way the jenkins jobs are set up.

  • Added shellcheck action because I use this locally and it helps when refactoring.
    • Excluded all non test scripts from checking, but it will
  • Fixed the scripts to make shellcheck happy
    • quoting
    • switch comparison to consistent for bash, if [ "${SG_TEST_X509:-}" == "true" -a "${COUCHBASE_SERVER_PROTOCOL}" != "couchbases" ]; then to use && instead of -a
    • switch GO_TEST_FLAGS to use a bash array so that it could be correctly quoted to appease shellcheck
  • add REQUIRED_VARS check in the case I have to run the script locally, so it runs faster
  • Play with -x / +x so that it doesn't output all the test [ ] and is easy to read. I used the output to debug things but I think there's enough information to debug. This might just add noise.
  • Remove GO_MODULES and XATTRS since GO_MODULES isn't relevant since 3.1 and XATTRS is no longer relevant as of 4.0. This script now is always sourced from the repo, so I'm not worried about changes.
  • Remove unused DOCKER_CBS_ROOT_DIR since /workspace is the only volume used for cbcollects.
  • Remove \t from stdbuf -oL grep -a -E '(--- (FAIL|PASS|SKIP):|github.com/couchbase/sync_gateway(/.+)?\t|TEST: |panic: )' since grep always complainted about it, and it didn't do what it said. The scripts still find the values we need for the ok or failed for finishing package.

Testing

  • ran ./jenkins-integration-build.sh -m locally to simulate MasterIntegration job.
  • use ./start_server.sh locally (I already use this to create testing)

Integration Tests

Copilot AI review requested due to automatic review settings July 25, 2025 12:24
Copy link
Contributor

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 cleans up and modernizes Jenkins shell scripts by improving their reliability, maintainability, and tooling. The changes focus on removing deprecated variables, improving shell script best practices, and adding linting support.

Key changes:

  • Removed deprecated variables (GO_MODULES, XATTRS, DOCKER_CBS_ROOT_DIR) and simplified configuration
  • Enhanced shell script quality with shellcheck compliance (proper quoting, array usage, conditional operators)
  • Added GitHub Actions workflow for automated shellcheck linting

Reviewed Changes

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

Show a summary per file
File Description
jenkins-integration-build.sh Major cleanup removing deprecated vars, adding validation, improving shell compliance
integration-test/start_server.sh Removed unused DOCKER_CBS_ROOT_DIR variable and improved error handling
integration-test/service-test.sh Fixed shebang from #/bin/sh to #!/bin/bash
integration-test/docker-compose.yml Removed unused volume mount for DOCKER_CBS_ROOT_DIR
.github/workflows/shell.yml Added new shellcheck workflow for automated shell script linting


if [ "${DETECT_RACES:-}" == "true" ]; then
GO_TEST_FLAGS="${GO_TEST_FLAGS} -race"
GO_TEST_FLAGS=(-race)
Copy link

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

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

Setting GO_TEST_FLAGS to only contain (-race) overwrites all previously set flags. This should use += to append the flag: GO_TEST_FLAGS+=(-race)

Suggested change
GO_TEST_FLAGS=(-race)
GO_TEST_FLAGS+=(-race)

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment was an accurate one FWIW

@torcolvin torcolvin force-pushed the ci-improve-shell-scripts branch from 495af3a to 9b1f844 Compare September 12, 2025 01:19
@torcolvin torcolvin force-pushed the ci-improve-shell-scripts branch from 2b7ae0b to 67a1331 Compare September 30, 2025 14:43
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

few comments and the EditorConfig conflict from #7787 (Sorry)


if [ "${DETECT_RACES:-}" == "true" ]; then
GO_TEST_FLAGS="${GO_TEST_FLAGS} -race"
GO_TEST_FLAGS=(-race)
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment was an accurate one FWIW

# Test to see if Couchbase Server is up
# Each retry min wait 5s, max 10s. Retry 20 times with exponential backoff (delay 0), fail at 120s
curl --retry-all-errors --connect-timeout 5 --max-time 10 --retry 20 --retry-delay 0 --retry-max-time 120 'http://127.0.0.1:8091'
docker exec couchbase curl --fail --silent --retry-all-errors --connect-timeout 5 --max-time 10 --retry 20 --retry-delay 0 --retry-max-time 120 'http://127.0.0.1:8091'
Copy link
Member

Choose a reason for hiding this comment

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

why docker exec curl here and not on line 88?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I probably hit this with accidental debugging, I'd rather have it call curl from inside the container than outside given we don't know the network constraints.

@bbrks bbrks merged commit 08889a9 into main Oct 3, 2025
49 checks passed
@bbrks bbrks deleted the ci-improve-shell-scripts branch October 3, 2025 09:32
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.

3 participants