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

chore: remove duplicate 'GenerateNPods' #1252

Merged
merged 9 commits into from
Dec 10, 2020

Conversation

Yiyiyimu
Copy link
Member

@Yiyiyimu Yiyiyimu commented Dec 5, 2020

Signed-off-by: yiyiyimu wosoyoung@gmail.com

What problem does this PR solve?

There are two GenerateNPods exists with little difference, so merge them together

func generateNPods(

func GenerateNPods(

What is changed and how does it work?

Add a struct so don't need to type in the arguments not used in certain test.

Checklist

Tests

  • Unit test
  • E2E test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Breaking backward compatibility

Related changes

  • Need to update the documentation

Does this PR introduce a user-facing change?

NONE

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
@Yiyiyimu Yiyiyimu mentioned this pull request Dec 5, 2020
15 tasks
@codecov-io
Copy link

codecov-io commented Dec 5, 2020

Codecov Report

Merging #1252 (46f2bef) into master (7e9ff3f) will decrease coverage by 0.68%.
The diff coverage is 52.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1252      +/-   ##
==========================================
- Coverage   55.78%   55.09%   -0.69%     
==========================================
  Files          68       81      +13     
  Lines        4383     4993     +610     
==========================================
+ Hits         2445     2751     +306     
- Misses       1768     1981     +213     
- Partials      170      261      +91     
Impacted Files Coverage Δ
api/v1alpha1/common_types.go 0.00% <0.00%> (ø)
api/v1alpha1/common_webhook.go 100.00% <ø> (ø)
api/v1alpha1/dnschaos_type.go 0.00% <0.00%> (ø)
api/v1alpha1/dnschaos_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/httpchaos_types.go 0.00% <0.00%> (ø)
api/v1alpha1/iochaos_types.go 0.00% <ø> (-40.00%) ⬇️
api/v1alpha1/jvmchaos_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/kernelchaos_types.go 0.00% <ø> (-20.00%) ⬇️
api/v1alpha1/kernelchaos_webhook.go 100.00% <ø> (+14.81%) ⬆️
api/v1alpha1/kinds.go 27.27% <ø> (+0.60%) ⬆️
... and 123 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 3642d6b...46f2bef. Read the comment docs.

Copy link
Member

@STRRL STRRL left a comment

Choose a reason for hiding this comment

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

rest LGTM.

Since we are build test cases, explicitly environment is very important, so using default values is not so good for the situation.

And suggestions for default value: do not apply default value as if value == "" { value = defaultValue }; using a constructor to setup default value;

pkg/utils/generate.go Show resolved Hide resolved
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
pkg/utils/generate.go Outdated Show resolved Hide resolved
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
fewdan
fewdan previously requested changes Dec 8, 2020
@@ -0,0 +1,111 @@
// Copyright 2019 Chaos Mesh Authors.
Copy link
Member

Choose a reason for hiding this comment

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

s/2019/2020/

Copy link
Member Author

Choose a reason for hiding this comment

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

thx! fixed

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Copy link
Member

@STRRL STRRL 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

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 1221

@ti-srebot
Copy link
Contributor

/run-all-tests

@YangKeao
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@cwen0 cwen0 requested a review from fewdan December 10, 2020 11:28
@ti-srebot ti-srebot merged commit 5697f53 into chaos-mesh:master Dec 10, 2020
STRRL pushed a commit to STRRL/chaos-mesh that referenced this pull request Dec 15, 2020
Signed-off-by: STRRL <str_ruiling@outlook.com>
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

7 participants