Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge ci from main #2584

Merged
merged 9 commits into from
Nov 5, 2024
Merged

Conversation

avifenesh
Copy link
Collaborator

@avifenesh avifenesh commented Nov 3, 2024

Issue: #2548

This pull request introduces significant improvements to the CI/CD workflow documentation and configuration. The key changes include a comprehensive guide to the CI/CD workflow, updates to the build and engine matrices, and enhancements to the workflow templates. These changes aim to streamline the testing process and ensure consistent execution across different environments and languages.

Documentation Enhancements:

  • Added a detailed CI/CD Workflow Guide to .github/DEVELOPER.md, covering workflow triggers, test coverage, language-specific workflows, shared components, and dynamic matrix generation.

Configuration Updates:

  • Updated build-matrix.json to include new fields for specifying languages and added support for Amazon Linux. [1] [2]
  • Modified engine-matrix.json to include new engine versions and types, ensuring specific versions are always tested.
  • Added supported-languages-versions.json to define supported language versions and always-run versions for various languages.

Workflow Template Improvements:

  • Enhanced the create-test-matrices/action.yml to dynamically generate matrices based on inputs, optimizing the CI/CD process.
  • Updated csharp.yml to utilize the new dynamic matrices and added support for workflow dispatch inputs. [1] [2]

Minor Adjustments:

  • Simplified the build-node-wrapper/action.yml and build-python-wrapper/action.yml by adjusting the npm install process and making the engine version input optional. [1] [2]
  • Improved the pull request template checklist formatting.

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
  • Commits will be squashed upon merging.

@avifenesh avifenesh requested a review from a team as a code owner November 3, 2024 21:05
@avifenesh avifenesh force-pushed the merge_CI_from_main branch 5 times, most recently from b3c1255 to 7ae99a6 Compare November 4, 2024 10:21
@avifenesh avifenesh force-pushed the merge_CI_from_main branch 3 times, most recently from e167018 to bf1e3f7 Compare November 4, 2024 14:06
@Yury-Fridlyand
Copy link
Collaborator

Yury-Fridlyand commented Nov 5, 2024

As it was discussed with the team, this PR is too big and contains changes from multiple PRs. It is definetely recommended to split it into multiple PRs and iteratively increase the test coverage.

Split PR

Split PR into multiple PRs. Some of them could be published and reviewed in parallel. Steps to do there.

  1. Post and merge first PR, which contains CI changes only: .github/{json_matrices,workflows,DEVELOPER.md}. This will all CI backbone, but tests executed on every PR will remain the same. Scheduled job won't run on this branch. We can trigger it manually, but there is no reason until test fixed.
  2. Merge test fixes for redis 6 (task Fix tests on redis 6 #2524): Java: Fix script kill IT #2523, Python: Fix CI on redis 6 #2535, Node: Fix IT on redis 6 #2536.
  3. Manually run jobs on all redis/valkey versions.
  4. PR(s) with additional fixes for python (for 3.13 support), node (dependency update).
  5. Post a PR with the rest test fixes if needed.
  6. (optional, but recommended) Add CI fixes to use cache for rust lib build and valkey/redis build (tasks CI: cache rust compiled binaries #771/Enhance CI time - Rust component. #1962 and Caching ValKey for self-hosted runners. #2544). This will speed up the full CI runs for at least an hour.
  7. Reorganize test strategy and matrices. I'll describe this below.

Once step 5 completed we will have a full matrix CI green. Keep in mind that it still takes a lot of time and may suffer from unexpected CI or flakey test.

Known issues as of now (at least ones I'm aware about):

  1. The repo has only 1 linux arm runner. GH-hosted linux arm runners are more expensive than others, so it the repo (or the org maybe) doesn't use them. So repo has only 1 runner of such type and it has very high occupancy.
  2. Each job rebuilds the client from the scratch, and valkey/redis too, it takes ~1.5 min for each action. There are tasks to introduce/fix the cache use to speed up this.

Reorganize testing.

Running all tests on all platform on all language/framework versions on all server versions on ... etc ... isn't a good approach. Adding a small 5 sec test increases overall CI duration for few minutes. Adding a new server version will add few hours there.
Let me list few facts there and then I'll add proposals.

  1. There is completely no reason to repeat all tests no all supported java versions for java client - we don't use java 11+ features (our min supported version) nor we don't have code like "if java 17 do this else do that". Same applicable for all other clients.
  2. There is no reason to repeat all tests on all platform except platform specific ones.
  3. There is no reason to repeat all tests on all server versions. All client mechanisms are version insensitive.
  4. In 95% we're trying to test the server instead of testing the client.

Proposal:

  1. Run all tests on one most common platform on most recent server version only. Linux x64 - Valkey 8.
  2. Run sanity/smoke tests on all platforms on different (or random) server versions. Mininal benchmark is a good enough as a sanity test there. We should also add there pubsub, script, cluster scan tests.
  3. Run benchmark on different language/framework versions to cover that.

Following that proposal, full matrix tests won't run longer than an hour. We will be able to run full testing job on all release branches every night for all clients.

It is not too late to do this.

avifenesh and others added 7 commits November 5, 2024 05:59
* Update README.md

Signed-off-by: Avi Fenesh <[email protected]>

* Update README.md - lint fix

Signed-off-by: Avi Fenesh <[email protected]>

* Update README.md ling

Signed-off-by: Avi Fenesh <[email protected]>

* Update node/README.md

Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Avi Fenesh <[email protected]>

---------

Signed-off-by: Avi Fenesh <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
* CI - Minimal and full CI matrix impl

Signed-off-by: avifenesh <[email protected]>

* Fix mypy failing (valkey-io#2453)

---------

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: avifenesh <[email protected]>

* Python: adds JSON.ARRLEN command (valkey-io#2403)

---------

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: avifenesh <[email protected]>

---------

Signed-off-by: avifenesh <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Co-authored-by: Shoham Elias <[email protected]>
* CI - Minimal and full CI matrix impl

Signed-off-by: avifenesh <[email protected]>

* Fix mypy failing (valkey-io#2453)

---------

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: avifenesh <[email protected]>

* Python: adds JSON.ARRLEN command (valkey-io#2403)

---------

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: avifenesh <[email protected]>

---------

Signed-off-by: avifenesh <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Co-authored-by: Shoham Elias <[email protected]>
* CI - Minimal and full CI matrix impl

Signed-off-by: avifenesh <[email protected]>

* Fix mypy failing (valkey-io#2453)

---------

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: avifenesh <[email protected]>

* Python: adds JSON.ARRLEN command (valkey-io#2403)

---------

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: avifenesh <[email protected]>

---------

Signed-off-by: avifenesh <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Co-authored-by: Shoham Elias <[email protected]>
)

* Refactor tests to use async cleanup and improve error handling

Signed-off-by: avifenesh <[email protected]>

* Enhance Jest configuration and add test setup file; update build scripts and dependencies

Signed-off-by: avifenesh <[email protected]>

* Update devDependencies in package.json for hybrid-node-tests to latest versions

Signed-off-by: avifenesh <[email protected]>

* Enhance test utilities and command tests with improved wait logic and version checks

Signed-off-by: avifenesh <[email protected]>

* Refactor tests to assert expected replica reads are less than or equal to actual reads; update connection handling in utilities and allow unused imports in types

Signed-off-by: avifenesh <[email protected]>

* Update dependencies and enhance PyO3 bindings; add new features and improve type handling

Signed-off-by: avifenesh <[email protected]>

* Update GitHub workflows: enhance linting configurations, adjust engine version requirements, and remove obsolete Redis installation workflow

Signed-off-by: avifenesh <[email protected]>

---------

Signed-off-by: avifenesh <[email protected]>
@avifenesh
Copy link
Collaborator Author

@Yury-Fridlyand
I get your intention, but we need to do fixes on release, and we need to unblock the team to make them able to go back to work.
Plus — team got a list of errors and pushed them into release, and we want to run the action on release. We don't have the luxury of rearranging the branch into separate PRs and taking it through a month of reviewing and fixing.
The first step, as agreed, is merging, with red CI, and from there start running fixes.
Next point, probably part of 1.3 — creating a plan to make CI work as needed.
Right now — make 1.2 possible by making the release branch green. To do so, we need to merge this into the release branch and run the actions on the release branch in loops.

@avifenesh avifenesh force-pushed the merge_CI_from_main branch 2 times, most recently from c16a373 to 31303a4 Compare November 5, 2024 09:00
@avifenesh avifenesh force-pushed the merge_CI_from_main branch 3 times, most recently from 7236ee1 to f2f7abf Compare November 5, 2024 09:14
@avifenesh avifenesh merged commit 2b6578a into valkey-io:release-1.2 Nov 5, 2024
54 of 62 checks passed
@avifenesh avifenesh deleted the merge_CI_from_main branch November 5, 2024 09:29
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