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

Refactoring internal/db/kvs/redis package #590

Merged
merged 6 commits into from
Jul 30, 2020

Conversation

hlts2
Copy link
Collaborator

@hlts2 hlts2 commented Jul 21, 2020

Description:

I refactored internal/db/kvs/redis package. Because I can not add test.
In this PR contains changes of core logic and test.
I fixed the logic with author, so I think the merge is fine without confirmation of author.

Related Card:

https://github.com/vdaas/vald/projects/3#card-42173491

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.14.4
  • Docker Version: 19.03.8
  • Kubernetes Version: 1.18.2
  • NGT Version: 1.12.0

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

@pull-assistant
Copy link

pull-assistant bot commented Jul 21, 2020

Score: 0.70

Best reviewed: commit by commit


Optimal code review plan (3 warnings)

fix: refactoring

...nternal/db/kvs/redis/option.go 62% changes removed in fix: feedback of aut...

fix: apply gotests

...nal/db/kvs/redis/redis_test.go 73% changes removed in fix: feedback of aut...

feat: test code

...nternal/db/kvs/redis/option.go 67% changes removed in fix: feedback of aut...

...nal/db/kvs/redis/redis_test.go 45% changes removed in fix: feedback of aut...

     fix: feedback of author

     fix: apply suggestion

     🤖 Update license headers / Format go codes and yaml files

Powered by Pull Assistant. Last update 0b721b9 ... 11417fc. Read the comment docs.

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - add changelog comment
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase master

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #590 into master will increase coverage by 4.36%.
The diff coverage is 28.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage    7.84%   12.21%   +4.36%     
==========================================
  Files         388      407      +19     
  Lines       19945    21236    +1291     
==========================================
+ Hits         1565     2594    +1029     
- Misses      18156    18360     +204     
- Partials      224      282      +58     
Impacted Files Coverage Δ
internal/cache/cacher/cacher.go 100.00% <ø> (+100.00%) ⬆️
internal/client/discoverer/discover.go 0.00% <0.00%> (ø)
internal/compress/gob.go 26.19% <0.00%> (-1.31%) ⬇️
internal/compress/gzip.go 26.98% <0.00%> (-7.02%) ⬇️
internal/compress/lz4.go 36.36% <0.00%> (-5.31%) ⬇️
internal/compress/zstd.go 29.50% <0.00%> (-5.79%) ⬇️
internal/config/blob.go 0.00% <0.00%> (ø)
internal/config/config.go 15.78% <ø> (+15.78%) ⬆️
internal/config/grpc.go 0.00% <0.00%> (ø)
internal/config/log.go 100.00% <ø> (+100.00%) ⬆️
... and 162 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d8eb92...11417fc. Read the comment docs.

@github-actions github-actions bot added size/XL and removed size/L labels Jul 21, 2020
@hlts2 hlts2 changed the title [WIP] Refactoring internal/db/kvs/redis package Refactoring internal/db/kvs/redis package Jul 21, 2020
@kevindiu
Copy link
Contributor

/rebase

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by kevindiu for branch: refactoring/internal/redis

@vdaas-ci vdaas-ci force-pushed the refactoring/internal/redis branch from e021cf1 to cc032ed Compare July 28, 2020 09:29
type args struct {
ctx context.Context
}
type fields struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
struct of size 240 bytes could be of size 232 bytes (maligned)

@kevindiu
Copy link
Contributor

LGTM

kevindiu
kevindiu previously approved these changes Jul 28, 2020
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

@hlts2 hlts2 requested a review from vankichi July 28, 2020 10:31
vankichi
vankichi previously approved these changes Jul 29, 2020
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

internal/db/kvs/redis/redis.go Outdated Show resolved Hide resolved
internal/db/kvs/redis/redis.go Outdated Show resolved Hide resolved
@vankichi
Copy link
Contributor

@hlts2 when you fix, I'll merge it.

@hlts2
Copy link
Collaborator Author

hlts2 commented Jul 30, 2020

/rebase

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by hlts2 for branch: refactoring/internal/redis

kevindiu
kevindiu previously approved these changes Jul 30, 2020
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

vankichi
vankichi previously approved these changes Jul 30, 2020
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

@vankichi
Copy link
Contributor

/rebase
/format
/approve

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by vankichi for branch: refactoring/internal/redis

@vdaas-ci
Copy link
Collaborator

[FORMAT] Updating license headers and formatting go codes triggered by vankichi.

@vdaas-ci vdaas-ci dismissed stale reviews from vankichi and kevindiu via 11417fc July 30, 2020 06:39
Copy link
Collaborator

@vdaas-ci vdaas-ci left a comment

Choose a reason for hiding this comment

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

[APPROVED] This PR is approved by vankichi.

@vankichi vankichi merged commit f793475 into master Jul 30, 2020
@vankichi vankichi deleted the refactoring/internal/redis branch July 30, 2020 07:53
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.

4 participants