Skip to content

Conversation

hanxizh9910
Copy link
Contributor

@hanxizh9910 hanxizh9910 commented Sep 10, 2025

Problem

When releasing new changes, all Docker images are rebuilt even when only one version changes. This wastes significant time and resources.

Solution

This PR adds change detection to the CI workflow and detect which version folder changed.

Testing

Verified the optimization works for:

  • ✅ Single version changes (builds only that version)
  • ✅ Multi-version changes (builds only changed versions)
  • ✅ Documentation changes (builds nothing)

@Nikhil-Manglore Nikhil-Manglore self-requested a review September 11, 2025 00:16
Signed-off-by: Hanxi Zhang <[email protected]>
Copy link
Collaborator

@sarthakaggarwal97 sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

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

Minor comments. I am interested in how we are testing this. Let's add some workflows and tests through maybe personal forks.

@hanxizh9910
Copy link
Contributor Author

hanxizh9910 commented Oct 9, 2025

Hi @sarthakaggarwal97, here are the screenshots showing how I tested the changes. I’ve also linked the PR from my personal fork.

Expected Behavior

The expected behavior is as follows: when pushing to the mainline branch, only the version whose Dockerfile has been modified should be built. If no Dockerfile is changed, no image should be built. For pushes to other branches or for pull requests, all jobs should run.

Only modified 8.1 dockerfile: hanxizh9910#20

PR:

Screenshot 2025-10-08 at 5 08 32 PM

Push to mainline:

Screenshot 2025-10-08 at 5 09 48 PM

Modified both 8.1 and 9.0:hanxizh9910#21

PR:

Screenshot 2025-10-08 at 5 20 21 PM

Push to mainline:

Screenshot 2025-10-08 at 5 16 06 PM

Modified the ci.yml(not a dockerfile):hanxizh9910#22

PR:

Screenshot 2025-10-08 at 5 20 21 PM

Push to mainline:

Screenshot 2025-10-08 at 5 20 46 PM

Signed-off-by: Sarthak Aggarwal <[email protected]>
Copy link
Collaborator

@sarthakaggarwal97 sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @hanxizh9910 for taking this up and testing it. @roshkhatri and @Nikhil-Manglore please take a look. It'll be good to have this for 9.0 GA.

@sarthakaggarwal97
Copy link
Collaborator

@roshkhatri I will merge this EOD :D

@Nikhil-Manglore
Copy link
Member

Yeah it looks good to me

Copy link
Member

@roshkhatri roshkhatri left a comment

Choose a reason for hiding this comment

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

LGTM

echo "strategy=$(jq -c . <<<"$strategy")" >> "$GITHUB_OUTPUT"
jq . <<<"$strategy" # debugging aid to visually inspect the generated matrix
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this line?

Signed-off-by: Roshan Khatri <[email protected]>
@roshkhatri roshkhatri merged commit 2c8e1e5 into valkey-io:mainline Oct 13, 2025
2 of 10 checks passed
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