Skip to content

Conversation

@jduo
Copy link
Collaborator

@jduo jduo commented Oct 31, 2025

  • Add support for finding the native lib in Windows.
  • Support integration tests in Windows using an external Valkey instance configured remotely.
  • Add CI integration to run Windows in a self-hosted runner against an external self-hosted Valkey Linux server.
  • Add remote_cluster_manager.py for managing version-specific installation, starting, and stopping of the
    clusters on the external Valkey server. Update gradle and ValkeyCluster.java to use this. It depends on environment
    variables VALKEY_REMOTE_HOST and SSH_PRIVATE_KEY_CONTENT.

Issue link

This Pull Request is linked to issue (URL): [#3780]

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

@jduo jduo requested a review from a team as a code owner October 31, 2025 19:46
Copy link
Collaborator

@ikolomi ikolomi left a comment

Choose a reason for hiding this comment

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

Guys, I am really lost here -

I dont understand the reason behind going with a persistent self-hosted windows and linux runners for CI tests and engine backends - It is complicated (you introduce the whole docs for creds and setup), has a state (which can drift) requires dealing with security tokens, introduces network aspects and most importantly hard to scale.

We already have an ephemeral infrastructure in place for linux and macs, we should reuse and adapt it.
There are a number of better options which wont require all this complexity in the source code - for example, we have a bare metal macos, which is able to spin ephemeral VMs to run the macos tests, we can extend this pattern to run windows desktops vms with local docker inside for running redis/valkey backends.

Another option - use ephemeral windows ec2 for running the test but modify cluster_manager to spin a backend in a fargate (very localized change)

We already have all the pieces and knowledge to handle it in the AWS infrastructure, but instead we are moving away from it with this PR. This is really complicated and definitely and overengineered.

{
"OS": "windows",
"NAMED_OS": "windows",
"RUNNER": ["self-hosted", "windows", "x64"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

why to use self-hosted? - this is last ditch resolve when where is no runners of github

# Add authentication header if token is available
if [ -n "$GITHUB_TOKEN" ]; then
curl -H "Authorization: Bearer $GITHUB_TOKEN" -LO $PB_REL/download/v29.1/protoc-29.1-linux-x86_64.zip
Copy link
Collaborator

Choose a reason for hiding this comment

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

why needed?

if [[ "$EVENT_NAME" == "pull_request" || "$EVENT_NAME" == "push" || "$RUN_FULL_MATRIX" == "false" ]]; then
echo 'Pick engines marked as `"run": "always"` only - on PR, push or manually triggered job which does not require full matrix'
jq -c '[.[] | select(.run == "always")]' < .github/json_matrices/engine-matrix.json | awk '{ printf "engine-matrix=%s\n", $0 }' | tee -a $GITHUB_OUTPUT
ENGINES=$(jq -c '[.[] | select(.run == "always")]' < .github/json_matrices/engine-matrix.json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why isnt awk required? and why to touch it anyway?

echo 'Getting "always run" runners'
BASE_MATRIX=$(jq -c '[.[] | select(.run == "always")]' < .github/json_matrices/build-matrix.json)
echo 'Getting "always run" runners for this language'
BASE_MATRIX=$(jq --arg lang "$LANGUAGE_NAME" -c '[.[] | select(.run == "always" and .languages? and any(.languages[] == $lang; .) and '"$CONDITION"')]' < .github/json_matrices/build-matrix.json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to change the conditions? - it should work for java on windows as added in build-matrix.json...

// Add basic changelog information
const changelog = `## Changelog\n\nUpdated ${depName} from ${fromVersion} to ${toVersion}\n\n📋 To view detailed changes, visit the package repository or release notes.`;
const changelog = `## Changelog\n\nUpdated ${depName} from ${fromVersion} to ${toVersion}\n\n[INFO] To view detailed changes, visit the package repository or release notes.`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why required in this PR?

shell: bash
env:
RC_VERSION: ${{ github.event.inputs.rc-version }}
CARGO_BUILD_JOBS: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

what for?

ENGINES=$(jq -c . < .github/json_matrices/engine-matrix.json)
fi
# Note: Redis 6.2 exclusion for Windows is handled in the host matrix filtering
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should have it here (since its the centralized script that builds the permutations) instead having it in the specific language CI script...

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe better yet - "run: always" should be removed from the engine-matrix.json for 6.2, because it is not run always anymore

host: ${{ fromJson(needs.get-matrices.outputs.host-matrix-output) }}
exclude:
# Exclude Redis 6.2 on Windows (not supported)
- engine: { type: "redis", version: "6.2" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not to filter in the create-test-matricies?

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.

4 participants