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

[update] add client.Reader in Reconciler #892

Merged
merged 12 commits into from
Sep 10, 2020
Merged

[update] add client.Reader in Reconciler #892

merged 12 commits into from
Sep 10, 2020

Conversation

fewdan
Copy link
Member

@fewdan fewdan commented Sep 9, 2020

Signed-off-by: “fewdan” [email protected]

What problem does this PR solve?

solve #878

What is changed and how does it work?

add client.Reader in Reconciler

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: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
@fewdan fewdan added status/PTAL type/bug-fix Fix for a previously reported bug. labels Sep 9, 2020
yeya24
yeya24 previously approved these changes Sep 9, 2020
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Overall this looks perfect! Thanks for the quick fix.
Another thing is that can you please add some E2E tests to ensure this working as expected with fieldSelector?

pkg/utils/selector.go Show resolved Hide resolved
pkg/apiserver/common/common.go Outdated Show resolved Hide resolved
Signed-off-by: “fewdan” <[email protected]>
@fewdan
Copy link
Member Author

fewdan commented Sep 10, 2020

@yeya24 I have dealt with the other comments, but the e2e test seems inconvenient to add. I tested it manually before.

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.

rest LGTM

pkg/utils/selector.go Outdated Show resolved Hide resolved
YangKeao
YangKeao previously approved these changes Sep 10, 2020
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

ti-srebot
ti-srebot previously approved these changes Sep 10, 2020
@YangKeao
Copy link
Member

Passing an argument to every function is really annoying (:crying_cat_face: I need implicit). Maybe we should think about the code structure and the way to refine it. But for this PR, it's enough to merge 👍 .

@fewdan
Copy link
Member Author

fewdan commented Sep 10, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@fewdan merge failed.

@fewdan fewdan dismissed stale reviews from ti-srebot and YangKeao via 1e9eb38 September 10, 2020 11:57
Signed-off-by: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
Signed-off-by: “fewdan” <[email protected]>
@chaos-mesh chaos-mesh deleted a comment from ti-srebot Sep 10, 2020
@chaos-mesh chaos-mesh deleted a comment from ti-srebot Sep 10, 2020
@fewdan
Copy link
Member Author

fewdan commented Sep 10, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

LGTM

@fewdan
Copy link
Member Author

fewdan commented Sep 10, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@fewdan fewdan merged commit d94435f into chaos-mesh:master Sep 10, 2020
@fewdan fewdan deleted the testyihaofu branch September 10, 2020 12:30
STRRL pushed a commit to STRRL/chaos-mesh that referenced this pull request Sep 10, 2020
* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* fix vet

Signed-off-by: “fewdan” <[email protected]>

* fix go vet

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* address comments

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* address comment

Signed-off-by: “fewdan” <[email protected]>
@ti-srebot
Copy link
Contributor

cherry pick to release-1.0 failed

@Yiyiyimu Yiyiyimu mentioned this pull request Oct 2, 2020
4 tasks
sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021
* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* fix vet

Signed-off-by: “fewdan” <[email protected]>

* fix go vet

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* address comments

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* update

Signed-off-by: “fewdan” <[email protected]>

* address comment

Signed-off-by: “fewdan” <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge status/LGT3 type/bug-fix Fix for a previously reported bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants