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

Add E2E tests for read replica feature #2298

Merged
merged 22 commits into from
Jan 22, 2024
Merged

Add E2E tests for read replica feature #2298

merged 22 commits into from
Jan 22, 2024

Conversation

ykadowak
Copy link
Contributor

@ykadowak ykadowak commented Jan 19, 2024

Description:

This PR adds E2E for read replica as TestE2EReadReplica.

  • Add minikube as e2e env because read replica needs ROX policy that k3d does not support.
  • Update some e2e k8s client helper
  • Implement the e2e test logic
    • deploy vald and vald-readreplica -> insert -> save index -> rotate read replica -> search

Related Issue:

Versions:

  • Go Version: 1.21.5
  • Docker Version: 20.10.8
  • Kubernetes Version: v1.28.4
  • NGT Version: 2.1.6

Checklist:

Special notes for your reviewer:

Copy link

cloudflare-pages bot commented Jan 19, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0df5a99
Status: ✅  Deploy successful!
Preview URL: https://ae66e4d7.vald.pages.dev
Branch Preview URL: https://feature-readreplica-e2e.vald.pages.dev

View logs

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@vdaas-ci
Copy link
Collaborator

[WARNING:INTCFG] Changes in interal/config may require you to change Helm charts. Please check.

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e25aba1) 30.15% compared to head (0df5a99) 30.14%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2298      +/-   ##
==========================================
- Coverage   30.15%   30.14%   -0.02%     
==========================================
  Files         389      389              
  Lines       40826    40826              
==========================================
- Hits        12313    12307       -6     
- Misses      27861    27867       +6     
  Partials      652      652              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added size/L and removed size/L labels Jan 22, 2024
@ykadowak ykadowak changed the title [WIP] Feature/readreplica/e2e Add E2E tests for read replica feature Jan 22, 2024
@ykadowak ykadowak marked this pull request as ready for review January 22, 2024 07:46
@ykadowak ykadowak requested review from a team, vankichi and hlts2 and removed request for a team January 22, 2024 07:47
@@ -104,14 +106,12 @@ func init() {
flag.Parse()

var err error
kubeClient, err = client.New(*kubeConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, is it necessary that the environment in which the test is run always have information about the connection to the Kubernetes cluster? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

TestE2EReadReplica needs to be dependent on k8s client anyway so I guess I should revert this and check this in TestE2EReadReplica? That way other e2e tests can run without k8s clinet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed that way ed4e8b3 since I think it's better anyway.

Let me know if this is not what you intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

so I guess I should revert this and check this in TestE2EReadReplica?

Thank you for reply 🙏
Yes, I think that would be better!!

Copy link
Contributor

Choose a reason for hiding this comment

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

I reviewed your changes! It looks good to me! 🙏
Thank you 🙏

@vankichi
Copy link
Contributor

I will LGTM if the above conversation has been resolved

ykadowak and others added 3 commits January 22, 2024 08:48
This commit fixes the style issues introduced in d127dbc according to the output
from Gofumpt and Prettier.

Details: #2298
Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants