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

Unify fault injection commands #356

Merged
merged 5 commits into from
Oct 23, 2023
Merged

Conversation

pablochacin
Copy link
Collaborator

Description

Change the way port mapping is managed in service fault injection to allow reusing the pod injection logic.

Fixes #261 (no longer required)

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant integration test locally (make integration-xxx for affected packages)
  • I have run relevant e2e test locally (make e2e-xxx for disruptors, or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Copy link
Collaborator

@roobre roobre left a comment

Choose a reason for hiding this comment

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

I love how simpler this looks and how you figured out how to compose the Service and Pod disruptors.

Looking great, left a few comments mostly about nits and naming, should not be blockers.

pkg/api/convert.go Outdated Show resolved Hide resolved
pkg/types/intstr/intstr.go Outdated Show resolved Hide resolved
pkg/types/intstr/intstr.go Outdated Show resolved Hide resolved
pkg/types/intstr/intstr.go Outdated Show resolved Hide resolved
pkg/types/intstr/intstr_test.go Outdated Show resolved Hide resolved
pkg/types/intstr/intstr.go Show resolved Hide resolved
pkg/utils/kubernetes.go Outdated Show resolved Hide resolved
pkg/types/intstr/intstr.go Outdated Show resolved Hide resolved
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Copy link
Collaborator

@roobre roobre left a comment

Choose a reason for hiding this comment

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

Left a comment on a potential typo, but otherwise LGTM! Feel free to merge right away after resolving the comment.

@@ -17,6 +20,7 @@ import (
// string <-- string
// time.Duration <-- string
// time.Time <-- string (only in RFC3339 format)
// IntOrStr <-- string or int64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// IntOrStr <-- string or int64
// IntOrStr <-- string or int32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not a typo. In JS the only int type supported in int64 the API layer will convert to int32 and throws an error if the value overflows. I added a comment to clarify this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does goja support int64s? I though all JS numbers were spec'd as 64b IEEE floats, but I'm not sure how goja does it here.

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
@pablochacin pablochacin merged commit 75eb36e into main Oct 23, 2023
11 of 15 checks passed
@pablochacin pablochacin deleted the unify-fault-injection-commands branch October 23, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests for fault injection in Service Disruptor
2 participants