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

Fix to create index_path when it does not exists #2060

Merged
merged 7 commits into from
Jun 14, 2023
Merged

Conversation

ykadowak
Copy link
Contributor

@ykadowak ykadowak commented Jun 14, 2023

Description:

This PR fixes the regression that introduced in #2034.

When index_path does not exist, it used to create the directory and #2034 made it an error. This PR reverts it.

Related Issue:

Versions:

  • Go Version: 1.20.3
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 2.0.11

Checklist:

Special notes for your reviewer:

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase main
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 38.46% and project coverage change: -0.03 ⚠️

Comparison is base (ad3507e) 29.93% compared to head (d9ff733) 29.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
- Coverage   29.93%   29.90%   -0.03%     
==========================================
  Files         369      369              
  Lines       35090    35092       +2     
==========================================
- Hits        10504    10495       -9     
- Misses      24104    24115      +11     
  Partials      482      482              
Impacted Files Coverage Δ
internal/errors/agent.go 92.68% <0.00%> (-7.32%) ⬇️
internal/errors/errors.go 50.24% <ø> (+0.72%) ⬆️
pkg/agent/core/ngt/service/ngt.go 35.51% <50.00%> (+0.10%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 14, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d9ff733
Status: ✅  Deploy successful!
Preview URL: https://4ed3c7a6.vald.pages.dev
Branch Preview URL: https://fix-agent-broken-index.vald.pages.dev

View logs

@ykadowak ykadowak changed the title [WIP] stop checking if n.path exists Fix to create index_path when it does not exists Jun 14, 2023
@ykadowak ykadowak requested review from kpango, a team and kevindiu and removed request for a team June 14, 2023 06:06
files, err := file.ListInDir(path)
if err != nil {
return err
return fmt.Errorf("failed to list in %v: %w", path, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In vald it would be better to define new error at internal/errors and use it.

Copy link
Contributor Author

@ykadowak ykadowak Jun 14, 2023

Choose a reason for hiding this comment

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

@ykadowak ykadowak requested a review from kpango June 14, 2023 06:25
kevindiu
kevindiu previously approved these changes Jun 14, 2023
Copy link
Contributor

@kevindiu kevindiu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vankichi vankichi left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit 02927a3 into main Jun 14, 2023
@kpango kpango deleted the fix/agent/broken-index branch June 14, 2023 09:12
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.

5 participants