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.

Sign up for GitHub

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

Jump to bottom

Set default value for correlation and jitter of NetworkChaos #361

Merged
merged 3 commits into from Mar 23, 2020
Merged

Set default value for correlation and jitter of NetworkChaos #361

merged 3 commits into from Mar 23, 2020

Conversation

Gallardot
Copy link
Member

@Gallardot Gallardot commented Mar 22, 2020 •
edited

UCP #292

What problem does this PR solve?

#292

What is changed and how does it work?

Check List

Tests

  • Unit test

Code changes

  • Has Go code change

Side effects

Related changes

Does this PR introduce a user-facing change?:

NONE

@Gallardot Gallardot changed the title set default value for correlation and jitter of NetworkChaos Set default value for correlation and jitter of NetworkChaos Mar 22, 2020
@codecov-io
Copy link

Codecov Report

Merging #361 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #361      +/-   ##
========================================
- Coverage      61%    61%   -0.01%     
========================================
  Files          56     56              
  Lines        3347   3354       +7     
========================================
+ Hits         2042   2046       +4     
- Misses       1145   1147       +2     
- Partials      160    161       +1
Impacted Files Coverage Δ
api/v1alpha1/networkchaos_types.go 13.08% <ø> (ø) ⬆️
api/v1alpha1/networkchaos_webhook.go 82.05% <100%> (+3.92%) ⬆️
pkg/utils/selector.go 51.65% <0%> (-0.83%) ⬇️
pkg/ptrace/ptrace_linux.go 65.55% <0%> (-0.38%) ⬇️

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 1c6a077...a4cf807. Read the comment docs.

YangKeao
YangKeao previously approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouqiang-cl
Copy link
Contributor

zhouqiang-cl commented Mar 23, 2020 •
edited

Did we need to update doc about network?

AngleNet
AngleNet requested changes Mar 23, 2020
View reviewed changes
api/v1alpha1/networkchaos_webhook.go Outdated
@@ -44,6 +52,20 @@ func (in *NetworkChaos) Default() {
in.Spec.Selector.DefaultNamespace(in.GetNamespace())
// the target's namespace selector
in.Spec.Target.TargetSelector.DefaultNamespace(in.GetNamespace())

in.defaultDelay()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add it to NetworkChaosSpec to keep same style as other's setting default ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this opinion @Gallardot

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion!All done!

@Yisaer
Copy link
Contributor

Yisaer commented Mar 23, 2020

I think it is necessary to add unit test to verify the validating and default webhook. This request is ok to me. I suggest to create issue to mention us for the unit test later.

@Gallardot
Copy link
Member Author

Did we need to update doc about network?

@zhouqiang-cl I have updated the doc.

@AngleNet
Copy link
Contributor

LGTM

@Gallardot
Copy link
Member Author

Gallardot commented Mar 23, 2020 •
edited

I think it is necessary to add unit test to verify the validating and default webhook. This request is ok to me. I suggest to create issue to mention us for the unit test later.

@Yisaer I have added some unit test for the validating and default webhook before. Maybe you means e2e test for the webhook?

@sre-bot
Copy link
Contributor

sre-bot commented Mar 23, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 604 points.

YangKeao
YangKeao approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

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

LGTM

@YangKeao
Copy link
Member

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 23, 2020

/run-all-tests

@sre-bot sre-bot merged commit 0039a69 into chaos-mesh:master Mar 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 23, 2020

Team Gallardot complete task #292 and get 604 score, currerent score 904

@Gallardot Gallardot deleted the delay-patch branch April 16, 2020 03:03
sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021
@Gallardot
Set default value for correlation and jitter of NetworkChaos (chaos-m…
5ace05b 
…esh#361)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwen0 cwen0 cwen0 left review comments

@YangKeao YangKeao YangKeao approved these changes

@AngleNet AngleNet AngleNet approved these changes

Assignees
No one assigned
Labels
component/operator status/can-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Gallardot @codecov-io @zhouqiang-cl @Yisaer @AngleNet @sre-bot @YangKeao @cwen0